Skip to content

Conversation

lpinca
Copy link
Member

@lpinca lpinca commented Apr 1, 2021

The socket might be destroyed by the other peer while data is still
being written. Add the missing error handler.

Fixes: #37291

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. labels Apr 1, 2021
@lpinca lpinca force-pushed the fix/issue-37291 branch from d0efa94 to 8241289 Compare April 1, 2021 13:31
client.on('error', function(err) {
// The socket might be destroyed by the other peer while data is still
// being written.
assert.strictEqual(err.code, 'ECONNRESET');
Copy link
Member Author

Choose a reason for hiding this comment

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

I think err.code can also be 'EPIPE' or 'ECONNABORTED' but I was not able to reproduce. Should we handle that in advance or do it if/when it will occur?

Copy link
Member

@Trott Trott Apr 3, 2021

Choose a reason for hiding this comment

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

I'd say let's do it when it occurs.

Copy link
Member

Choose a reason for hiding this comment

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

Perhaps you can add a comment right now that says "EPIPE might be valid too but we haven't seen it yet" or something like that?

@nodejs-github-bot
Copy link
Collaborator

@lpinca lpinca force-pushed the fix/issue-37291 branch from 8241289 to 93ce076 Compare April 3, 2021 05:27
The socket might be destroyed by the other peer while data is still
being written. Add the missing error handler.

Fixes: nodejs#37291
@lpinca lpinca force-pushed the fix/issue-37291 branch from 93ce076 to 316d8c7 Compare April 6, 2021 16:56
@nodejs-github-bot
Copy link
Collaborator

lpinca added a commit that referenced this pull request Apr 8, 2021
The socket might be destroyed by the other peer while data is still
being written. Add the missing error handler.

PR-URL: #38018
Fixes: #37291
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@lpinca
Copy link
Member Author

lpinca commented Apr 8, 2021

Landed in cc4ee6c.

@lpinca lpinca closed this Apr 8, 2021
@lpinca lpinca deleted the fix/issue-37291 branch April 8, 2021 17:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-ci PRs that need a full CI run. test Issues and PRs related to the tests.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

investigate flaky test/parallel/test-http-many-ended-pipelines.js

5 participants