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 intermitent condition where onerror is called when conn is closed by client #72

Closed

Conversation

ngbrown
Copy link
Contributor

@ngbrown ngbrown commented Jul 20, 2023

The test "onerror is not called when conn is closed by client" intermittently fails.

According to some resources, such as RFC-1122 (4.2.2.12 Closing a Connection): if the host actively closes a connection, while still having unread incoming data available, the host should send the signal RST (losing any received data) instead of FIN. This may or may not be what is happening, but there's there's something causing a RST to be received by the client at least some of the time.

This pull request proposes the solution of calling socket.destroy() in closeSocket() to ignore the possibility of a RST response. This was also the solution in postwait/node-amqp#444.

See recent past failures:

@dentarg
Copy link
Member

dentarg commented Jul 21, 2023

Nice, this gets rid of the amqp-client connection closed read ECONNRESET seen when running the README example #64 (comment)

@dentarg dentarg requested a review from baelter July 21, 2023 07:49
@dentarg dentarg closed this in 9f4d420 Jul 21, 2023
@dentarg dentarg added the bug Something isn't working label Jul 21, 2023
@dentarg
Copy link
Member

dentarg commented Jul 21, 2023

There should be an option to allow maintainers to push to your branches/PRs, then I can update (rebase) and merge them, so you don't have to do all that work, and will look like they was merged too. :)

@ngbrown
Copy link
Contributor Author

ngbrown commented Jul 21, 2023

There should be an option to allow maintainers to push to your branches/PRs, then I can update (rebase) and merge them, so you don't have to do all that work, and will look like they was merged too. :)

I've seen that option on Pull Requests in GitHub in the past, but it's not there on these PRs. It might be because I'm forking into an separate account/org?

@ngbrown ngbrown deleted the bug/onerror-when-closed branch July 21, 2023 14:34
@dentarg
Copy link
Member

dentarg commented Jul 21, 2023

I've seen that option on Pull Requests in GitHub in the past, but it's not there on these PRs.

I saw it working for another PR, it said (in the right side-bar) Maintainers are allowed to edit this pull request.

It might be because I'm forking into an separate account/org?

Sounds like that reading https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork

Who can use this feature

People with push access to the upstream repository of a fork owned 
by a personal account can commit to the forked branches.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants