-
Notifications
You must be signed in to change notification settings - Fork 30k
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 race in test-net-socket-local-address #4052
Conversation
test-net-socket-local-address had a race condition that resulted in unreliability on FreeBSD. This changes fixes the issue. Fixes: nodejs#2475
LGTM if CI is happy |
Here's 28 failures in 129 runs of the test without this fix on FreeBSD: https://ci.nodejs.org/job/node-test-commit-freebsd/437/nodes=freebsd102-64/console And here's 10000 successful runs without a single failure with this fix on FreeBSD: https://ci.nodejs.org/job/node-test-commit-freebsd/436/nodes=freebsd102-64/console |
LGTM. One less flaky test \o/ |
Looks like this one is also currently flaky on Windows, so this fix isn't just for FreeBSD's benefit. Woot. |
I don't understand where the race condition comes from. Does a 'close' event get lost somehow? |
@bnoordhuis Either a server |
Continues to afflict Windows without this fix: https://ci.nodejs.org/job/node-test-binary-windows/165/RUN_SUBSET=3,VS_VERSION=vs2015,label=win2012r2/tapTestReport/test.tap-142/ Guess I better run a stress test on Windows with this fix to make sure it works there too. (I had only run it on FreeBSD before.): https://ci.nodejs.org/job/node-stress-single-test/73/nodes=win2012r2/console |
Stress test worked out A-OK on Windows. Seems ready to land, unless... @bnoordhuis Was your question just a question or an expression of concern that my response doesn't adequately address? I'd like to land this if it was just the former, but will hold off if the latter. |
I had a look at the test and I believe there is a simpler way to fix the flakiness: #4078 |
Simpler solution in #4078 appears to solve the problem for Windows but not FreeBSD. Details over there. Maybe it just needs a small tweak or something. |
Alternate solution that doesn't involve creating a Boolean and adding special logic based on it: #4109 Yay, three competing PRs! |
Closing in favor of #4109 |
test-net-socket-local-address had a race condition that resulted in
unreliability on FreeBSD. This changes fixes the issue.
Fixes: #2475