test: fix flaky test-dgram-exclusive-implicit-bind by Trott · Pull Request #10212 · nodejs/node · GitHub

Checklist

make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

test dgram

Description of change

test-dgram-exclusive-implicit-bind is written assuming that dgram
messages are received with 100% reliability. While missing a dgram
message sent to localhost is rare, we do see it as evidenced by CI
failures from time to time.

The test has been rewritten to send dgram messages over and over until
the test requirements have been met.

Additional incidental refactoring includes:

var -> const
use of common.mustCall() instead of exit listener + boolean

Sample CI failure indicating this test was flaky:

https://ci.nodejs.org/job/node-test-commit-linux/6627/nodes=fedora24/console

not ok 1295 parallel/test-dgram-exclusive-implicit-bind
  ---
  duration_ms: 60.62
  severity: fail
  stack: |-
    timeout

CI: https://ci.nodejs.org/job/node-test-pull-request/5349/

Could try a stress test, but I don't think this happens with enough frequency for that to be useful without a very large number of runs. Here's master on fedora24 run 9999 times. We'll see if it fails at all. If so, we can do another run with this PR and see if it comes up clean.

https://ci.nodejs.org/job/node-stress-single-test/1070/nodes=fedora24/console

Could all of this just be simplified to:

setInterval(() => {
  source.send(Buffer.from('abc'), 0, 3, common.PORT, '127.0.0.1');
}, 1).unref();

I think you shouldn't have to worry about clearing the interval if you unref it. That would reduce some complexity.

I was concerned that competing 1ms interval timers on some operating systems could result in increased flakiness, but I didn't actually test it, so yeah, let's try that. :-D

If I don't keep the timer-clearing logic, this happens:

Error: Not running
    at Socket._healthCheck (dgram.js:527:11)
    at Socket.send (dgram.js:347:8)
    at Timeout.setInterval [as _onTimeout] (/Users/trott/io.js/test/parallel/test-dgram-exclusive-implicit-bind.js:104:10)
    at ontimeout (timers.js:365:14)
    at Timer.unrefdHandle (timers.js:471:5)
dgram.js:527
    throw new Error('Not running'); // error message from dgram_legacy.js
    ^

test-dgram-exclusive-implicit-bind is written assuming that dgram messages are received with 100% reliability. While missing a dgram message sent to localhost is rare, we do see it as evidenced by CI failures from time to time. The test has been rewritten to send dgram messages over and over until the test requirements have been met. Additional incidental refactoring includes: * var -> const * use of common.mustCall() instead of exit listener + boolean

Updated to use setInterval() per suggestion from @cjihrig.

CI again: https://ci.nodejs.org/job/node-test-pull-request/5352/

From a previous comment I'm not sure this was a problem but can't you get rid of this as the interval is already unrefed? At least locally it works for me

If I remove that line, the test still passes, but it produces a messy output that looks like it's failing:

$ ./node test/parallel/test-dgram-exclusive-implicit-bind.js
dgram.js:527
    throw new Error('Not running'); // error message from dgram_legacy.js
    ^

Error: Not running
    at Socket._healthCheck (dgram.js:527:11)
    at Socket.send (dgram.js:347:8)
    at Timeout.setInterval [as _onTimeout] (/Users/trott/io.js/test/parallel/test-dgram-exclusive-implicit-bind.js:104:10)
    at ontimeout (timers.js:365:14)
    at Timer.unrefdHandle (timers.js:471:5)
dgram.js:527
    throw new Error('Not running'); // error message from dgram_legacy.js
    ^

Error: Not running
    at Socket._healthCheck (dgram.js:527:11)
    at Socket.send (dgram.js:347:8)
    at Timeout.setInterval [as _onTimeout] (/Users/trott/io.js/test/parallel/test-dgram-exclusive-implicit-bind.js:104:10)
    at ontimeout (timers.js:365:14)
    at Timer.unrefdHandle (timers.js:471:5)
$

I'd prefer the processes clean up after themselves and not generate irrelevant errors like that, even if the errors don't cause the test to fail, so I'd prefer to keep the clearInterval() call.

Understood

LGTM with a suggestion / question

CI: https://ci.nodejs.org/job/node-test-pull-request/5359/

Landed 3ddbea6

test-dgram-exclusive-implicit-bind is written assuming that dgram messages are received with 100% reliability. While missing a dgram message sent to localhost is rare, we do see it as evidenced by CI failures from time to time. The test has been rewritten to send dgram messages over and over until the test requirements have been met. Additional incidental refactoring includes: * var -> const * use of common.mustCall() instead of exit listener + boolean PR-URL: #10212 Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com> Reviewed-By: Italo A. Casas <me@italoacasas.com>

test-dgram-exclusive-implicit-bind is written assuming that dgram messages are received with 100% reliability. While missing a dgram message sent to localhost is rare, we do see it as evidenced by CI failures from time to time. The test has been rewritten to send dgram messages over and over until the test requirements have been met. Additional incidental refactoring includes: * var -> const * use of common.mustCall() instead of exit listener + boolean PR-URL: nodejs#10212 Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com> Reviewed-By: Italo A. Casas <me@italoacasas.com>

test-dgram-exclusive-implicit-bind is written assuming that dgram messages are received with 100% reliability. While missing a dgram message sent to localhost is rare, we do see it as evidenced by CI failures from time to time. The test has been rewritten to send dgram messages over and over until the test requirements have been met. Additional incidental refactoring includes: * var -> const * use of common.mustCall() instead of exit listener + boolean PR-URL: #10212 Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com> Reviewed-By: Italo A. Casas <me@italoacasas.com>

Trott added dgram Issues and PRs related to the dgram subsystem / UDP. test Issues and PRs related to the tests. labels Dec 10, 2016

Trott requested a review from santigimeno December 10, 2016 06:49

nodejs-github-bot added dont-land-on-v7.x test Issues and PRs related to the tests. labels Dec 10, 2016

Trott requested a review from cjihrig December 10, 2016 06:53

cjihrig reviewed Dec 10, 2016

View reviewed changes

Trott added 2 commits December 10, 2016 14:23

squash: use setInterval instead of chaining setTimeouts

909e78e

Trott force-pushed the fix-implicit branch from c99d44e to 909e78e Compare December 10, 2016 22:50

nodejs-github-bot added the dont-land-on-v7.x label Dec 10, 2016

santigimeno reviewed Dec 11, 2016

View reviewed changes

squash: clearTimeout() -> clearInterval()

e346f84

nodejs-github-bot added the lts-watch-v6.x label Dec 11, 2016

santigimeno approved these changes Dec 11, 2016

View reviewed changes

Fishrock123 removed the dont-land-on-v7.x label Dec 12, 2016

italoacasas approved these changes Dec 13, 2016

View reviewed changes

italoacasas closed this Dec 13, 2016

italoacasas mentioned this pull request Dec 15, 2016

v7.3.0 proposal #10277

Merged

MylesBorins added the dont-land-on-v4.x label Jan 22, 2017

MylesBorins added land-on-v6.x and removed lts-watch-v6.x labels Jan 22, 2017

MylesBorins mentioned this pull request Jan 24, 2017

v6.10.0 proposal #10974

Merged

gibfahn mentioned this pull request Mar 28, 2017

Potential Semver Minor Backports nodejs/Release#188

Closed

31 tasks

Trott deleted the fix-implicit branch January 13, 2022 22:44

+              source.on('close', function() {
+                clearTimeout(interval);
+              });

test: fix flaky test-dgram-exclusive-implicit-bind by Trott · Pull Request #10212 · nodejs/node · GitHub

Uh oh!

Uh oh!

test: fix flaky test-dgram-exclusive-implicit-bind - #10212

test: fix flaky test-dgram-exclusive-implicit-bind#10212Trott wants to merge 3 commits intonodejs:masternodejs/node:masterfrom Trott:fix-implicitTrott/io.js:fix-implicitCopy head branch name to clipboard

Conversation

Trott commented Dec 10, 2016

Checklist

Affected core subsystem(s)

Description of change

Uh oh!

Trott commented Dec 10, 2016

Uh oh!

Trott commented Dec 10, 2016

Uh oh!

Trott commented Dec 10, 2016

Uh oh!

cjihrig Dec 10, 2016

Choose a reason for hiding this comment

Uh oh!

Trott Dec 10, 2016

Choose a reason for hiding this comment

Uh oh!

Trott Dec 10, 2016

Choose a reason for hiding this comment

Uh oh!

Trott commented Dec 10, 2016

Uh oh!

santigimeno Dec 11, 2016

Choose a reason for hiding this comment

Uh oh!

Trott Dec 11, 2016

Choose a reason for hiding this comment

Uh oh!

santigimeno Dec 11, 2016

Choose a reason for hiding this comment

Uh oh!

santigimeno left a comment

Choose a reason for hiding this comment

Uh oh!

Trott commented Dec 11, 2016

Uh oh!

italoacasas commented Dec 13, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

test: fix flaky test-dgram-exclusive-implicit-bind#10212
Trott wants to merge 3 commits into
nodejs:masterfrom
Trott:fix-implicit