-
Notifications
You must be signed in to change notification settings - Fork 476
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
Close and destroy underlying socket after any connection error #584
base: main
Are you sure you want to change the base?
Conversation
Thank you! Works great |
npm install squaremo/amqp.node#pull/584/head for those of you that want to use this PR in your apps |
Didn't know that installing versions from PRs was an option, and this not being merged was a stopper for me. Good to know! Just asking, @squaremo is this project not active anymore? |
@xabinapal from your perspective, is it possible to cover the code with tests? It would allow @squaremo to merge the PR without hesitation. |
I haven't thoroughly investigated how tests work in this library, only to ensure all test cases continue working as expected. I think it would be technically possible to add some tests to check that connection closes after an error. Anyways, this library seems to be 100% abandoned. So many issues and PRs without owner's feedback, and this #418 issue searching for maintainers is also quiet since 2018. With zero confidence on this PR being merged, I'm not willing to spend any time improving it, sorry. I'have been using it in production thanks to #584 (comment) with no problems, and I encourage everyone who needs this to use that method. If @squaremo or any other maintainer relaunches this project, I will gladly spend more time on this issue. |
The naming is strange because I don't want to introduce any breaking change. Note that jest complains about leaking resources due to: amqp-node/amqplib#584
The naming is strange because I don't want to introduce any breaking change. Note that jest complains about leaking resources due to: amqp-node/amqplib#584
The naming is strange because I don't want to introduce any breaking change. Note that jest complains about leaking resources due to: amqp-node/amqplib#584
The naming is strange because I don't want to introduce any breaking change. Note that jest complains about leaking resources due to: amqp-node/amqplib#584
@xabinapal I know that quite a bit of time has passed, but would you be open to updating this PR? Library is not quite dead these days :) |
Ensure that on connection errors (either from the underlying socket or from the AMQP protocol) no more messages are send to the destination and fully close and destroy the socket. This fixes the open socket leak after sending invalid connection options, such as invalid credentials or virtual host, for example.
Based on #514, resolves #513.