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

fix(microservices): adds feedback message when RabbitMQ server connection hangs #9751

Conversation

delucca
Copy link
Contributor

@delucca delucca commented Jun 10, 2022

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Other... Please describe:

What is the current behavior?

Issue Number: #9749

If a RabbitMQ connection fails, NestJS keeps trying to reconnect but no log message is dispatched. So, the user thinks the server frozen and it is not doing anything else.

What is the new behavior?

Upon a failed connection on the RabbitMQ server, I've added an error log to show what happened to the user.

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

At first we were considering that #9726 was also impacted by this issue, but it isn't, since this is specific to how RabbitMQ handles failed connection attempts.

@coveralls
Copy link

coveralls commented Jun 10, 2022

Pull Request Test Coverage Report for Build fe9290e2-9103-4c52-a729-c9dcd92384d8

  • 2 of 4 (50.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.03%) to 94.078%

Changes Missing Coverage Covered Lines Changed/Added Lines %
packages/microservices/server/server-rmq.ts 1 3 33.33%
Totals Coverage Status
Change from base Build 3833bc5d-ac71-4db9-b614-33791ff96fb8: -0.03%
Covered Lines: 5783
Relevant Lines: 6147

💛 - Coveralls

@delucca delucca changed the title fix(microservices): adds feedback message when microservice server connection hangs fix(microservices): adds feedback message when RabbitMQ server connection hangs Jun 10, 2022
@delucca delucca marked this pull request as ready for review June 10, 2022 21:24
@micalevisk
Copy link
Member

I wonder if wouldn't be better (more flexible) if we just expose the underlying server by returning this.server in ServerRMQ#start 🤔

@delucca
Copy link
Contributor Author

delucca commented Jun 12, 2022

I wonder if wouldn't be better (more flexible) if we just expose the underlying server by returning this.server in ServerRMQ#start 🤔

I'm not sure if that would solve the issue. Do you know if by returning it we would prevent the server retrying to start?

Because the current issue is that we're stuck in a loop were the server keeps trying to connection and the event we receive from the RabbitMQ client is not being handled by our server, so we don't get any log message in the terminal

@micalevisk
Copy link
Member

micalevisk commented Jun 12, 2022

Do you know if by returning it we would prevent the server retrying to start?

it still won't.

I'm unsure if there is any other way to listen to that connectFailed event. So what if we want to handle it instead of just logging it out? By returning the server we could handle that in our preferable way

@delucca
Copy link
Contributor Author

delucca commented Jun 12, 2022

Do you know if by returning it we would prevent the server retrying to start?

it still won't.

I'm unsure if there is any other way to listen to that connectFailed event. So what if we want to handle it instead of just logging it out? By returning the server we could handle that in our preferable way

I ran a few quick tests here, it seems the RabbitMQ client has this built-in feature of retrying the connection if it fails.

What do you think should be the optimal approach from NestJS perspective (ignoring RabbitMQ client)?

  • Upon a failed connection, we should log and error and continue with the application bootstrap, without retrying?
  • Upon a failed connection, we should log and error, continue to bootstrap the application and retry the connection on the background?
  • Upon a failed connection, we should log the error, retry N times and them end the process afterwards if the connection fails on all attempts?

I think how we handle this depends on that decision 🤔

@kamilmysliwiec
Copy link
Member

@delucca what you did here in this PR is correct and aligns with other transporters. Thank you!

@kamilmysliwiec kamilmysliwiec merged commit 279f6fa into nestjs:master Jun 14, 2022
@delucca delucca deleted the fix/server-transport-connection-hanging branch June 14, 2022 14:12
@avec-as
Copy link

avec-as commented Jul 18, 2022

Hello,
good to have more log, but not a good idea I think to directly log the error because it's log the entire connection object with the password in plain text !!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants