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 drop connection by RabbitMQ #120

Merged
merged 9 commits into from
May 19, 2024
Merged

Conversation

viras777
Copy link

@viras777 viras777 commented Nov 21, 2021

Fix if RabbitMQ closed the connection due to incorrect login/password.
Similar problems in here issues:
Change state to error when ClientException is throw #115
or
Failing connections cause exceptions thrown outside of Promise #74
or
ClientException #59

If RabbitMQ closed the connection due to incorrect login/password
If RabbitMQ closed the connection due to incorrect login/password
@WyriHaximus
Copy link
Collaborator

Thank you for filing this PR, could you rebase on the latest changes in master now that #106, and do you think it is possible to test this?

@viras777
Copy link
Author

I tested on my test server and now works on the production server

@WyriHaximus
Copy link
Collaborator

@viras777 Thanks for the update. Any clue what it fails for certain jobs?

@viras777
Copy link
Author

In the function awaitConnectionTune I added interception of all exceptions to be able to process in reject.
like this:
Connection = (new Client())->connect()->then()->otherwise();

Because of this, the tests do not pass, they expect another type of exception

separate into two different patches
Fix bug with missing heartbeat
@viras777
Copy link
Author

Sometimes there are missed hearbeats with an error on the rabbitmq server: "missing heartbeats from client, timeout: 60s" and bunny take 100 CPU for a while.
This happens when $nextStreamSelectTimeout < $ now < $ stopTime
For example call method run(1) and wait 60s

@WyriHaximus WyriHaximus added this to the v0.5.6 milestone May 19, 2024
Copy link
Collaborator

@WyriHaximus WyriHaximus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, sorry for the late merge. Planning to release this in a few days

@WyriHaximus WyriHaximus merged commit 4842701 into jakubkulhan:master May 19, 2024
@WyriHaximus
Copy link
Collaborator

Odd, CI blow up, but not on lowest possible dependencies. Will fix before releasing

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

Successfully merging this pull request may close these issues.

2 participants