nest icon indicating copy to clipboard operation
nest copied to clipboard

[BUG] ClientRMQ never asserts replyQueue if one is specified, manual ack impossible if noAck=true

Open constb opened this issue 3 years ago • 5 comments

Bug Report

Current behavior

I create a ClientRMQ instance for RMQ transport. I set replyQueue manually to the value different from built-in "direct reply queue", I also set noAck to true.

Input Code

ClientsModule.register([
  {
    name: 'WORKER_MQ',
    transport: Transport.RMQ,
    options: {
      urls: [process.env.RABBIT_MQ_URL || 'amqp://localhost'],
      queue: 'app.requests',
      replyQueue: 'app.replies',
      prefetchCount: 15,
      persistent: true,
      noAck: true,
      queueOptions: { durable: true },
    },
  },
]),

Expected behavior

I expect a request to be sent and then a reply to be received. I exprect to be able to ack the reply manually.

Unfortunatelly there's no way to manually ack a reply.

Also I get an error logged:

[2021-10-01 08:37:24.124 +0000] ERROR (app-rpc/30729 on MacBook-Pro-Konstantin.local):
    trace: "ClientProxy"
    msg: {
      "err": {
        "code": 404,
        "classId": 60,
        "methodId": 20
      }
    }
    reqId: "d687ca536742424fa94cc69c70b541fd"
    context: "AmqpConnectionManager.<anonymous>"
(node:30729) UnhandledPromiseRejectionWarning: Error: Operation failed: BasicConsume; 404 (NOT-FOUND) with message "NOT_FOUND - no queue 'app.replies' in vhost '/'"
    at reply /xxx/node_modules/.pnpm/[email protected]/node_modules/amqplib/lib/channel.js:134:29)
    at ConfirmChannel.C.accept (/xxx/node_modules/.pnpm/[email protected]/node_modules/amqplib/lib/channel.js:417:7)
    at Connection.mainAccept [as accept] (/xxx/node_modules/.pnpm/[email protected]/node_modules/amqplib/lib/connection.js:64:33)
    at Socket.go (/xxx/node_modules/.pnpm/[email protected]/node_modules/amqplib/lib/connection.js:478:48)
    at Socket.emit (events.js:400:28)
    at emitReadable_ (internal/streams/readable.js:550:12)
    at processTicksAndRejections (internal/process/task_queues.js:81:21)

Possible Solution

Since manuall ack on reply is not possible (if it even makes sense to do one at all), ClientRMQ should ignore noAck option, it should only be used by the ServerRMQ.

Since it makes sense to always use the "direct reply queue", replyQueue option should be removed (or ignored for backwards compatibility) as well. Otherwise, nest should assert reply queue before trying to consume it.

Environment

Nest version: 8.0.6
Node version: v14.17.6
Platform: Nest app on Mac, RabbitMQ 3.8.22 on Linux

constb avatar Oct 01 '21 09:10 constb

Would you like to create a PR for this issue?

kamilmysliwiec avatar Oct 04 '21 07:10 kamilmysliwiec

Would you like to create a PR for this issue?

I'd love to, but what do I do with replyQueue? Should it be asserted or deprecated?

constb avatar Oct 04 '21 08:10 constb

@constb your problem is that the reply queue is not automatically created.

Also, if you want to manually ack messages, you need to set noAck to false. The noAck parameter is the same as autoAck.

Personally, I use the special amq.rabbitmq.reply-to queue for direct replies, which you don't have to create manually. See: https://www.rabbitmq.com/direct-reply-to.html

Also, from what I'm reading there, the amq.rabbitmq.reply-to queue must be used in auto-ack mode. It is very useful and should not be deprecated (I use it).

However, if you need manual acknowledgement, you can ignore the replyQueue parameter and set noAck to false. Just ensure that you use ClientProxy#send() and @MessagePattern()

dgradwell-ams avatar Oct 20 '21 00:10 dgradwell-ams

@dgradwell-ams Yes, reply-to queue is the way to go with ClientRMQ, and the fact that Nest uses it (with auto ack) by default is great.

The problem is that ClientRMQ allows you to change those parameters but if you do, that will never work. First beause custom reply queue is not asserted, end second because there's no way to ack a response.

I would prefer to have both of those footguns removed from framework code if @kamilmysliwiec doesn't mind…

constb avatar Oct 21 '21 06:10 constb

@constb what you are proposing would be a major breaking change, as many people are likely using it the way it works now.

dgradwell-ams avatar Oct 21 '21 17:10 dgradwell-ams