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

test: fix tls-multi-key race condition #3966

Closed
wants to merge 1 commit into from

Conversation

santigimeno
Copy link
Member

In some conditions it can happen that the client-side socket is destroyed
before the server-side socket has gracefully closed, thus causing a
'ECONNRESET' error in this socket. To solve this, wait in the client-side
socket for the 'end' event before closing it.

It tries to fix the following error I've observed a couple of times running the tests:

Path: parallel/test-tls-multi-key
events.js:142
      throw er; // Unhandled 'error' event
      ^

Error: read ECONNRESET
    at exports._errnoException (util.js:873:11)
    at TLSWrap.onread (net.js:545:26)
Command: out/Release/node /Users/sgimeno/node/node/test/parallel/test-tls-multi-key.js

I've been able to reproduce it by increasing the data sent in the socket to around 10000 bytes

@mscdex mscdex added tls Issues and PRs related to the tls subsystem. test Issues and PRs related to the tests. labels Nov 22, 2015
@Mctalian
Copy link

Mctalian commented Dec 1, 2015

Is this at all related to #3595 or #3517 ?

@Trott
Copy link
Member

Trott commented Jan 13, 2016

Hmm, looks like this never got much in the way of eyes. @nodejs/crypto maybe?

@santigimeno Maybe rebase and force push just because it's been a while?

ciphers.push(rsa.getCipher());
ecdsa.destroy();
Copy link
Member

Choose a reason for hiding this comment

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

I think it should be safe to just replace destroy with end. Will the test pass with this?

@santigimeno
Copy link
Member Author

@indutny I've been trying to reproduce the error with the changes you propose with no luck. Do you want to create a new PR while I close this, or should I update this with your changes? Thanks!

@indutny
Copy link
Member

indutny commented Jan 15, 2016

I would appreciate if you will update this PR with these changes! You're the one who figured out the problem anyway!

In some conditions it can happen that the client-side socket is
destroyed before the server-side socket has gracefully closed, thus
causing a 'ECONNRESET' error in this socket. To solve this, also close
gracefully in the client side.
@santigimeno
Copy link
Member Author

PR updated. Thanks!

@Trott
Copy link
Member

Trott commented Jan 16, 2016

CI is green! https://ci.nodejs.org/job/node-test-pull-request/1277/

@indutny Does this look good to you as it is?

@indutny
Copy link
Member

indutny commented Jan 16, 2016

LGTM :D

@indutny
Copy link
Member

indutny commented Jan 16, 2016

Thank you!

@jasnell
Copy link
Member

jasnell commented Jan 18, 2016

LGTM

jasnell pushed a commit that referenced this pull request Jan 18, 2016
In some conditions it can happen that the client-side socket is
destroyed before the server-side socket has gracefully closed, thus
causing a 'ECONNRESET' error in this socket. To solve this, also close
gracefully in the client side.

PR-URL: #3966
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@jasnell
Copy link
Member

jasnell commented Jan 18, 2016

Landed in e65f1f7

@jasnell jasnell closed this Jan 18, 2016
evanlucas pushed a commit that referenced this pull request Jan 18, 2016
In some conditions it can happen that the client-side socket is
destroyed before the server-side socket has gracefully closed, thus
causing a 'ECONNRESET' error in this socket. To solve this, also close
gracefully in the client side.

PR-URL: #3966
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 28, 2016
In some conditions it can happen that the client-side socket is
destroyed before the server-side socket has gracefully closed, thus
causing a 'ECONNRESET' error in this socket. To solve this, also close
gracefully in the client side.

PR-URL: #3966
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 11, 2016
In some conditions it can happen that the client-side socket is
destroyed before the server-side socket has gracefully closed, thus
causing a 'ECONNRESET' error in this socket. To solve this, also close
gracefully in the client side.

PR-URL: #3966
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Feb 11, 2016
In some conditions it can happen that the client-side socket is
destroyed before the server-side socket has gracefully closed, thus
causing a 'ECONNRESET' error in this socket. To solve this, also close
gracefully in the client side.

PR-URL: nodejs#3966
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Feb 11, 2016
MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Feb 15, 2016
In some conditions it can happen that the client-side socket is
destroyed before the server-side socket has gracefully closed, thus
causing a 'ECONNRESET' error in this socket. To solve this, also close
gracefully in the client side.

PR-URL: nodejs#3966
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: James M Snell <[email protected]>
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
In some conditions it can happen that the client-side socket is
destroyed before the server-side socket has gracefully closed, thus
causing a 'ECONNRESET' error in this socket. To solve this, also close
gracefully in the client side.

PR-URL: nodejs#3966
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Issues and PRs related to the tests. tls Issues and PRs related to the tls subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants