nestjs / nest

A progressive Node.js framework for building efficient, scalable, and enterprise-grade server-side applications with TypeScript/JavaScript 🚀
https://nestjs.com
MIT License
67.08k stars 7.56k forks source link

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

Closed constb closed 1 year ago

constb commented 3 years ago

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/amqplib@0.8.0/node_modules/amqplib/lib/channel.js:134:29)
    at ConfirmChannel.C.accept (/xxx/node_modules/.pnpm/amqplib@0.8.0/node_modules/amqplib/lib/channel.js:417:7)
    at Connection.mainAccept [as accept] (/xxx/node_modules/.pnpm/amqplib@0.8.0/node_modules/amqplib/lib/connection.js:64:33)
    at Socket.go (/xxx/node_modules/.pnpm/amqplib@0.8.0/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
kamilmysliwiec commented 3 years ago

Would you like to create a PR for this issue?

constb commented 3 years ago

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?

dgradwell-ams commented 2 years ago

@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()

constb commented 2 years ago

@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…

dgradwell-ams commented 2 years ago

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