Skip to content

Conversation

@mscdex
Copy link
Contributor

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
@mscdexmscdex 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
ContributorAuthor

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

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 <[email protected]> Reviewed-By: João Reis <[email protected]>
@mscdex
Copy link
ContributorAuthor

Landed in 4b0b991.

@mscdexmscdex closed this Dec 19, 2015
@mscdexmscdex 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 <[email protected]> Reviewed-By: João Reis <[email protected]>
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 <[email protected]> Reviewed-By: João Reis <[email protected]>
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 <[email protected]> Reviewed-By: João Reis <[email protected]>
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 <[email protected]> Reviewed-By: João Reis <[email protected]>
@MylesBorinsMylesBorins 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 <[email protected]> Reviewed-By: João Reis <[email protected]>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

netIssues and PRs related to the net subsystem.testIssues 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

@mscdex@Trott@bnoordhuis@joaocgreis@jasnell@MylesBorins