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 flaky test-net-error-twice #4342

Closed
wants to merge 1 commit into from

Conversation

mscdex
Copy link
Contributor

@mscdex mscdex commented Dec 18, 2015

On Windows there can exist some race condition where the OS notification of the client's socket.destroy() isn't received before the server writes to the socket, so the OS happily writes to the server socket and the socket.destroy() notification is seen as an EOF. This race condition was more evident/reproducible on a single core system (for obvious reasons in hindsight).

This commit fixes the flakiness by waiting until the server's connection event handler has been called to destroy the client socket and perform the server socket write.

Fixes: #4057

On Windows there can exist some race condition where the
notification of the client's `socket.destroy()` isn't received
before the server writes to the socket. This race condition was
more evident/reproducible on a single core system.

This commit fixes the flakiness by waiting until the server's
connection event handler has been called to destroy the client
socket and perform the server socket write.

Fixes: nodejs#4057
@mscdex mscdex added net Issues and PRs related to the net subsystem. test Issues and PRs related to the tests. labels Dec 18, 2015
@mscdex
Copy link
Contributor Author

mscdex commented Dec 18, 2015

Stress test: https://ci.nodejs.org/job/node-stress-single-test/191/nodes=win2012r2/
CI is green except for another flaky test on Windows: https://ci.nodejs.org/job/node-test-commit/1464/

@Trott
Copy link
Member

Trott commented Dec 18, 2015

Works for me if it works for the others who have been looking at this:
R=@bnoordhuis
R=@joaocgreis

As I said in the other issue, I'd still like to know the mechanism by which adding console to an array in common.js triggers this issue. But that can be a separate bug/issue to track.

@bnoordhuis
Copy link
Member

LGTM

@joaocgreis
Copy link
Member

LGTM

The fact that this failed so reliably on Windows 2012 alone still worries me, together with the apparent interference with console.

mscdex added a commit that referenced this pull request Dec 19, 2015
On Windows there can exist some race condition where the
notification of the client's `socket.destroy()` isn't received
before the server writes to the socket. This race condition was
more evident/reproducible on a single core system.

This commit fixes the flakiness by waiting until the server's
connection event handler has been called to destroy the client
socket and perform the server socket write.

Fixes: #4057
PR-URL: #4342
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: João Reis <reis@janeasystems.com>
@mscdex
Copy link
Contributor Author

mscdex commented Dec 19, 2015

Landed in 4b0b991.

@mscdex mscdex closed this Dec 19, 2015
@mscdex mscdex deleted the fix-flaky-test-net-error-twice branch December 19, 2015 19:20
Fishrock123 pushed a commit to Fishrock123/node that referenced this pull request Dec 22, 2015
On Windows there can exist some race condition where the
notification of the client's `socket.destroy()` isn't received
before the server writes to the socket. This race condition was
more evident/reproducible on a single core system.

This commit fixes the flakiness by waiting until the server's
connection event handler has been called to destroy the client
socket and perform the server socket write.

Fixes: nodejs#4057
PR-URL: nodejs#4342
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: João Reis <reis@janeasystems.com>
Fishrock123 pushed a commit to Fishrock123/node that referenced this pull request Jan 6, 2016
On Windows there can exist some race condition where the
notification of the client's `socket.destroy()` isn't received
before the server writes to the socket. This race condition was
more evident/reproducible on a single core system.

This commit fixes the flakiness by waiting until the server's
connection event handler has been called to destroy the client
socket and perform the server socket write.

Fixes: nodejs#4057
PR-URL: nodejs#4342
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: João Reis <reis@janeasystems.com>
mscdex added a commit that referenced this pull request Jan 7, 2016
On Windows there can exist some race condition where the
notification of the client's `socket.destroy()` isn't received
before the server writes to the socket. This race condition was
more evident/reproducible on a single core system.

This commit fixes the flakiness by waiting until the server's
connection event handler has been called to destroy the client
socket and perform the server socket write.

Fixes: #4057
PR-URL: #4342
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: João Reis <reis@janeasystems.com>
MylesBorins pushed a commit that referenced this pull request Jan 19, 2016
On Windows there can exist some race condition where the
notification of the client's `socket.destroy()` isn't received
before the server writes to the socket. This race condition was
more evident/reproducible on a single core system.

This commit fixes the flakiness by waiting until the server's
connection event handler has been called to destroy the client
socket and perform the server socket write.

Fixes: #4057
PR-URL: #4342
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: João Reis <reis@janeasystems.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
On Windows there can exist some race condition where the
notification of the client's `socket.destroy()` isn't received
before the server writes to the socket. This race condition was
more evident/reproducible on a single core system.

This commit fixes the flakiness by waiting until the server's
connection event handler has been called to destroy the client
socket and perform the server socket write.

Fixes: nodejs#4057
PR-URL: nodejs#4342
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: João Reis <reis@janeasystems.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
net Issues and PRs related to the net subsystem. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Investigate flaky test-net-error-twice
6 participants