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

Close and destroy underlying socket after any connection error #584

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

xabinapal
Copy link

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.

@carlhoerberg
Copy link
Contributor

Thank you! Works great

@carlhoerberg
Copy link
Contributor

npm install squaremo/amqp.node#pull/584/head

for those of you that want to use this PR in your apps

@xabinapal
Copy link
Author

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?

@xamgore
Copy link
Contributor

xamgore commented Jan 19, 2021

@xabinapal from your perspective, is it possible to cover the code with tests? It would allow @squaremo to merge the PR without hesitation.

@xabinapal
Copy link
Author

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.

Base automatically changed from master to main February 17, 2021 22:01
luddd3 added a commit to luddd3/node-amqp-connection-manager that referenced this pull request Aug 27, 2021
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
luddd3 added a commit to luddd3/node-amqp-connection-manager that referenced this pull request Aug 27, 2021
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
luddd3 added a commit to luddd3/node-amqp-connection-manager that referenced this pull request Aug 30, 2021
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
luddd3 added a commit to luddd3/node-amqp-connection-manager that referenced this pull request Aug 30, 2021
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
@kibertoad
Copy link
Collaborator

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

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

Successfully merging this pull request may close these issues.

Failed "connect" call does not close socket
4 participants