Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Heartbeat for RMQ socket options not working as expected #5788

Closed
christianallred opened this issue Nov 25, 2020 · 5 comments
Closed

Heartbeat for RMQ socket options not working as expected #5788

christianallred opened this issue Nov 25, 2020 · 5 comments
Labels
needs triage This issue has not been looked into

Comments

@christianallred
Copy link
Contributor

christianallred commented Nov 25, 2020

Bug Report

Current behavior

When configuring a Rmq Microservice. using the documentation on the website: https://docs.nestjs.com/microservices/rabbitmq

It suggests that the socket options being used are those in the amqlib library, however. Upon inspecting the NestJS microservice factory it appears to be using the amqp-connection-manager library. Which has a different socketOptions footprint.
https://github.com/nestjs/nest/blob/master/packages/microservices/server/server-rmq.ts#L54

the socket options footprint is described here: https://www.npmjs.com/package/amqp-connection-manager#user-content-amqpconnectionmanagercreatechanneloptions

The documentation that is listed on nestJS says you should us heartbeat:60 per the documents in amqlib however teh amqp-connection-manager says to use heartbeatIntervalInSeconds: 60

Also, it would appear that neither heartbeat or heartbeatIntervalInSeconds actually has an effect on the heartbeat setting from what i can see.

The only way i was able to get the heartbeat to work as expected was to pass in the url as a object config instead as suggested by this PR: #5587

Input Code

app.connectMicroservice(<RmqOptions>{
    transport: Transport.RMQ,
    options: {
      urls: ['connectionstring'],
      queue: queue,
      socketOptions: {
        heartbeatIntervalInSeconds: 60,
      },
    },
  })

Expected behavior

  1. Need to validate what library is being used for socket options so we can update the documentation properly
  2. need to ensure that either amqplib, or amqp-connection-manager options are being respected properly when using connectMicroservice

Possible Solution

Update the documenation.

I am going to see if i can get the heartbeat config to work properly using what i see in the live code today: amqp-connection-manager

Environment


Nest version: ~7.0.13

 
For Tooling issues:
- Node version: v12.5.0
- Platform:  Mac

Others:

@christianallred christianallred added the needs triage This issue has not been looked into label Nov 25, 2020
@christianallred
Copy link
Contributor Author

christianallred commented Nov 25, 2020

It looks like the code calling amqp-connection-manager is passing over the options incorrectly:
https://github.com/jwalton/node-amqp-connection-manager

The Nest RMQ Factory is passing over the entire options object as {connectionOptions: options} but the amqp-connection-manager expects it in this format

`
options.heartbeatIntervalInSeconds

options.reconnectTimeInSeconds

options.findServers(callback)

options.connectionOptions
`

christianallred added a commit to christianallred/nest that referenced this issue Nov 25, 2020
@christianallred
Copy link
Contributor Author

#5790

@kamilmysliwiec
Copy link
Member

Thanks for reporting! Lets track this here #5790

christianallred added a commit to christianallred/nest that referenced this issue Dec 8, 2020
@arturgrigor
Copy link

In the latest versions of the @nestjs/microservices package, certain client options are not being properly passed to the AmqplibConnectionManager. Specifically, the options heartbeatIntervalInSeconds and reconnectTimeInSeconds are omitted when configuring the connection. Currently, only the connectionOptions property is being forwarded.

Issue in code: The problem can be traced to this section of the client-rmq.ts file in the Nest.js repository: client-rmq.ts#L139
Relevant connection manager code: The related code in AmqpConnectionManager can be found here: AmqpConnectionManager.ts#L200

cc @kamilmysliwiec

v-sum added a commit to v-sum/nest that referenced this issue Oct 8, 2024
all other properties, beside 'connectionOptions', from the socketOptions object ( of type AmqpConnectionManagerSocketOptions )
were being discarded on the creation of the AmqpConnectionManager client
https://github.com/nestjs/nest/blob/master/packages/microservices/external/rmq-url.interface.ts#L47
https://github.com/jwalton/node-amqp-connection-manager/blob/v4.1.14/src/AmqpConnectionManager.ts#L46

nestjs#5788 (comment)
@v-sum
Copy link
Contributor

v-sum commented Oct 8, 2024

@arturgrigor I've added a possible fix that would include the discarded options https://github.com/nestjs/nest/pull/14059/files

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs triage This issue has not been looked into
Projects
None yet
Development

No branches or pull requests

4 participants