test: fix flaky test-tls-socket-close#11921
Uh oh!
There was an error while loading. Please reload this page.
Conversation
Replace timer/timeout race with event-based ordering, eliminating test flakiness. Fixes: nodejs#11912
Trott added test 
Trott commented Mar 19, 2017
Stress test on current master (should show failures): https://ci.nodejs.org/job/node-stress-single-test-pi1-binary/17/label=pi1-raspbian-wheezy/console Stress test on this PR (should show no failures): https://ci.nodejs.org/job/node-stress-single-test-pi1-binary/19/label=pi1-raspbian-wheezy/console |
Trott commented Mar 19, 2017
Trott commented Mar 19, 2017
Argh! This version of the test doesn't fail/segfault on v7.0.0 the way the original version does, so I've now made the test invalid. Will mark this as stalled until I get around to fixing it or I give up and close it. |
Trott commented Mar 19, 2017
Fixed it, but now have to re-run all the stress tests etc.... |
Trott commented Mar 19, 2017
Current master stress test showing 28 failures in 999 runs: https://ci.nodejs.org/job/node-stress-single-test-pi1-binary/17/label=pi1-raspbian-wheezy/console |
Trott commented Mar 19, 2017
Stress test against this PR that will hopefully show no failures: https://ci.nodejs.org/job/node-stress-single-test-pi1-binary/21/label=pi1-raspbian-wheezy/console EDIT: Oof, this made it a lot worse. Back to the drawing board. |
Trott commented Mar 19, 2017
OK, did some step-debugging and hopefully this will now do it... Stress test against this PR: https://ci.nodejs.org/job/node-stress-single-test-pi1-fanned/25/ |
Trott commented Mar 19, 2017
Argh, cleaned up some unused code. Once more with feeling: Stress test against this PR: https://ci.nodejs.org/job/node-stress-single-test-pi1-fanned/26/ |
Trott commented Mar 19, 2017
All tests passing including stress test. |
jBarz commented Mar 19, 2017
LGTM apart from one nit. Thanks for fixing this! |
| // this breaks if TLSSocket is already managing the socket: | ||
| netSocket.destroy(); | ||
| constinterval=setInterval(()=>{ | ||
| // Checking this way allows us to do the right at a time that causes a |
Replace timer/timeout race with event-based ordering, eliminating test flakiness. PR-URL: nodejs#11921Fixes: nodejs#11912 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Santiago Gimeno <[email protected]> Reviewed-By: James M Snell <[email protected]>
Trott commented Mar 22, 2017
Landed in 7bc893f |
Replace timer/timeout race with event-based ordering, eliminating test flakiness. PR-URL: #11921Fixes: #11912 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Santiago Gimeno <[email protected]> Reviewed-By: James M Snell <[email protected]>
Replace timer/timeout race with event-based ordering, eliminating test flakiness. PR-URL: #11921Fixes: #11912 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Santiago Gimeno <[email protected]> Reviewed-By: James M Snell <[email protected]>
Replace timer/timeout race with event-based ordering, eliminating test flakiness. PR-URL: #11921Fixes: #11912 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Santiago Gimeno <[email protected]> Reviewed-By: James M Snell <[email protected]>
Replace timer/timeout race with event-based ordering, eliminating test flakiness. PR-URL: nodejs/node#11921Fixes: nodejs/node#11912 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Santiago Gimeno <[email protected]> Reviewed-By: James M Snell <[email protected]>
7 participants
Replace timer/timeout race with event-based ordering, eliminating test
flakiness.
Fixes: #11912
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
test tls