Skip to content

Fix intermitent condition where onerror is called when conn is closed by client#72

Closed
ngbrown wants to merge 1 commit intocloudamqp:mainfrom
ngbrown-forks:bug/onerror-when-closed
Closed

Fix intermitent condition where onerror is called when conn is closed by client#72
ngbrown wants to merge 1 commit intocloudamqp:mainfrom
ngbrown-forks:bug/onerror-when-closed

Conversation

@ngbrown
Copy link
Copy Markdown
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:

@ngbrown ngbrown force-pushed the bug/onerror-when-closed branch from 3e95f50 to cb48bbe Compare July 20, 2023 17:57
@dentarg
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

Reviewers

@baelter baelter baelter approved these changes

Assignees

No one assigned

Labels

bug Something isn't working

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@ngbrown @dentarg @baelter