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-inception #4195

Closed
wants to merge 2 commits into from
Closed

Conversation

santigimeno
Copy link
Member

Make sure all the data is read before checking its validity.
Remove gotHello variable and just check that the ssl end event
is received.

/cc @indutny

@JungMinu JungMinu added the test Issues and PRs related to the tests. label Dec 8, 2015
@mscdex mscdex added the tls Issues and PRs related to the tls subsystem. label Dec 8, 2015
@@ -13,7 +13,6 @@ var path = require('path');
var net = require('net');

var options, a, b, portA, portB;
Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW portA and portB can be removed too, they're not used.

@mscdex
Copy link
Contributor

mscdex commented Dec 8, 2015

LGTM

@indutny
Copy link
Member

indutny commented Dec 8, 2015

One nit from @mscdex , otherwise LGTM. Thanks!

Make sure all the data is read before checking its validity.
Remove `gotHello` variable and just check that the ssl `end` event
is received.
Remove unused variables.
When sending a very large buffer (400000 bytes) the test fails due to
the client socket from the `a` server erroring with `ECONNRESET`.
There's a race condition between the closing of this socket and the `ssl`
socket closing on the other side of the connection. To improve things,
destroy the socket as soon as possible: in the `end` event of the `dest`
socket.
@santigimeno
Copy link
Member Author

Updated the PR removing the unused variables.

I have also added a fix for the problems reported here #1012 (comment). I have sent this in a separate commit. Is this correct? or should I squash both commits into a single one? or a different PR?

/cc @indutny @mscdex


var body = new Buffer(4000).fill('A');
var body = new Buffer(400000).fill('A');
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 this is a bit over the top, probably? What do you think about a compromise with a 40000?

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've used that value to reproduce the ECONNRESET error in OS X consistently.

@indutny
Copy link
Member

indutny commented Dec 10, 2015

LGTM, thank you!

@indutny
Copy link
Member

indutny commented Dec 10, 2015

@indutny
Copy link
Member

indutny commented Dec 10, 2015

Unrelated test failures, landing!

@indutny
Copy link
Member

indutny commented Dec 10, 2015

Landed in 86a3bd0 and 3b94991. Thank you!

@indutny indutny closed this Dec 10, 2015
indutny pushed a commit that referenced this pull request Dec 10, 2015
Make sure all the data is read before checking its validity.
Remove `gotHello` variable and just check that the ssl `end` event
is received.
Remove unused variables.

PR-URL: #4195
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Fedor Indutny <fedor@indutny.com>
indutny pushed a commit that referenced this pull request Dec 10, 2015
When sending a very large buffer (400000 bytes) the test fails due to
the client socket from the `a` server erroring with `ECONNRESET`.
There's a race condition between the closing of this socket and the `ssl`
socket closing on the other side of the connection. To improve things,
destroy the socket as soon as possible: in the `end` event of the `dest`
socket.

PR-URL: #4195
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Fedor Indutny <fedor@indutny.com>
@santigimeno santigimeno deleted the tls_inception branch December 10, 2015 09:26
@santigimeno
Copy link
Member Author

Thanks

cjihrig pushed a commit that referenced this pull request Dec 15, 2015
Make sure all the data is read before checking its validity.
Remove `gotHello` variable and just check that the ssl `end` event
is received.
Remove unused variables.

PR-URL: #4195
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Fedor Indutny <fedor@indutny.com>
cjihrig pushed a commit that referenced this pull request Dec 15, 2015
When sending a very large buffer (400000 bytes) the test fails due to
the client socket from the `a` server erroring with `ECONNRESET`.
There's a race condition between the closing of this socket and the `ssl`
socket closing on the other side of the connection. To improve things,
destroy the socket as soon as possible: in the `end` event of the `dest`
socket.

PR-URL: #4195
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Fedor Indutny <fedor@indutny.com>
@rvagg rvagg mentioned this pull request Dec 17, 2015
MylesBorins pushed a commit that referenced this pull request Dec 30, 2015
Make sure all the data is read before checking its validity.
Remove `gotHello` variable and just check that the ssl `end` event
is received.
Remove unused variables.

PR-URL: #4195
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Fedor Indutny <fedor@indutny.com>
MylesBorins pushed a commit that referenced this pull request Dec 30, 2015
When sending a very large buffer (400000 bytes) the test fails due to
the client socket from the `a` server erroring with `ECONNRESET`.
There's a race condition between the closing of this socket and the `ssl`
socket closing on the other side of the connection. To improve things,
destroy the socket as soon as possible: in the `end` event of the `dest`
socket.

PR-URL: #4195
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Fedor Indutny <fedor@indutny.com>
MylesBorins pushed a commit that referenced this pull request Jan 19, 2016
Make sure all the data is read before checking its validity.
Remove `gotHello` variable and just check that the ssl `end` event
is received.
Remove unused variables.

PR-URL: #4195
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Fedor Indutny <fedor@indutny.com>
MylesBorins pushed a commit that referenced this pull request Jan 19, 2016
When sending a very large buffer (400000 bytes) the test fails due to
the client socket from the `a` server erroring with `ECONNRESET`.
There's a race condition between the closing of this socket and the `ssl`
socket closing on the other side of the connection. To improve things,
destroy the socket as soon as possible: in the `end` event of the `dest`
socket.

PR-URL: #4195
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Fedor Indutny <fedor@indutny.com>
@MylesBorins MylesBorins mentioned this pull request Jan 19, 2016
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
Make sure all the data is read before checking its validity.
Remove `gotHello` variable and just check that the ssl `end` event
is received.
Remove unused variables.

PR-URL: nodejs#4195
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Fedor Indutny <fedor@indutny.com>
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
When sending a very large buffer (400000 bytes) the test fails due to
the client socket from the `a` server erroring with `ECONNRESET`.
There's a race condition between the closing of this socket and the `ssl`
socket closing on the other side of the connection. To improve things,
destroy the socket as soon as possible: in the `end` event of the `dest`
socket.

PR-URL: nodejs#4195
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Fedor Indutny <fedor@indutny.com>
jBarz added a commit to ibmruntimes/node that referenced this pull request Feb 19, 2017
cherry-picked the following from master

commit 3b94991
Author: Santiago Gimeno <santiago.gimeno@gmail.com>
Date:   Tue Dec 8 14:57:22 2015 +0100

    test: fix tls-inception flakiness

    When sending a very large buffer (400000 bytes) the test fails due
    to the client socket from the `a` server erroring with `ECONNRESET`.
    There's a race condition between the closing of this socket and the
    `ssl` socket closing on the other side of the connection. To improve
    things, destroy the socket as soon as possible: in the `end` event
    of the `dest` socket.

    PR-URL: nodejs/node#4195
    Reviewed-By: Brian White <mscdex@mscdex.net>
    Reviewed-By: Fedor Indutny <fedor@indutny.com>
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.

6 participants