test: update test-timers-block-eventloop.js #16314

zhangzifa · 2017-10-19T09:08:58Z

When CPU is busy, the above sequential case fails occasionally,
expand the timeout value to fix it.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

lpinca · 2017-10-19T12:29:39Z

test/sequential/test-timers-block-eventloop.js

@@ -11,7 +11,7 @@ const t2 = setInterval(() => {
  common.busyLoop(15);
 }, 10);

-const t3 = setTimeout(common.mustNotCall('eventloop blocked!'), 100);
+const t3 = setTimeout(common.mustNotCall('eventloop blocked!'), 200);


Nit: it might make sense to use common.platformTimeout() everywhere and keep the original values. Not sure if it is enough though.

I'd also prefer common.platformTimeout() if that works.

I tested it with common.platformTimeout just now. Behavior is the same with 100. I will update the case with 200 and common.platformTimeout togegher.

When CPU is busy, the above sequential case fails occasionally, expand the timeout value to fix it. Fixes: nodejs#16310

gibfahn · 2017-10-20T12:17:16Z

test/sequential/test-timers-block-eventloop.js

-  common.busyLoop(12);
-}, 10);
+  common.busyLoop(commonTimeout(12));
+}, common.platformTimeout(10));


For consistency, maybe use commonTimeout() here too.

lpinca · 2017-10-20T12:30:43Z

test/sequential/test-timers-block-eventloop.js


 const t1 = setInterval(() => {
-  common.busyLoop(12);
-}, 10);
+  common.busyLoop(commonTimeout(12));


~~I'm not sure we should use common.platformTimeout() here. I think we don't want to increase the duration of the "busy" loop.~~

Edit: nvm this is probably correct or we will test something different from the original.

refack · 2017-10-27T20:30:28Z

Stress:
_{master: https://ci.nodejs.org/job/node-stress-single-test/1469/nodes=aix61-ppc64/

this: https://ci.nodejs.org/job/node-stress-single-test/1470/nodes=aix61-ppc64/

triggered AIX by mistake}
Master: https://ci.nodejs.org/job/node-stress-single-test/1471/nodes=rhel72-s390x/
This: https://ci.nodejs.org/job/node-stress-single-test/1472/nodes=rhel72-s390x/

joyeecheung · 2017-10-28T12:01:12Z

The stress tests did not reproduce the flakiness. Should we still merge this?

Also this just showed up in a Windows CI, and I am fairly certain that I had seen this on other platforms in the past week when I took a stroll in the CI:

https://ci.nodejs.org/job/node-test-binary-windows/12274/COMPILED_BY=vs2017,RUNNER=win2016,RUN_SUBSET=1/console

not ok 510 sequential/test-timers-block-eventloop
  ---
  duration_ms: 0.293
  severity: fail
  stack: |-
    assert.js:42
      throw new errors.AssertionError({
      ^
    
    AssertionError [ERR_ASSERTION]: eventloop blocked!
        at Timeout.mustNotCall [as _onTimeout] (c:\workspace\node-test-binary-windows\test\common\index.js:584:12)
        at ontimeout (timers.js:478:11)
        at tryOnTimeout (timers.js:302:5)
        at Timer.listOnTimeout (timers.js:262:5)
  ...

refack · 2017-10-28T22:20:23Z

The stress tests did not reproduce the flakiness. Should we still merge this?

Probably stress wasn't rigorous enough. Seems like test is just too sensitive. I confirmed test after change fails 100% on pre-patched version.

P.S. I've also seens this on https://ci.nodejs.org/job/node-test-binary-arm/11283/RUN_SUBSET=3,label=pi3-raspbian-jessie/console

refack · 2017-10-28T22:24:01Z

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

When CPU is busy, the above sequential case fails occasionally, expand the timeout value to fix it. PR-URL: nodejs#16314 Fixes: nodejs#16310 Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>

refack · 2017-10-29T16:47:05Z

Landed in 83b8474 (nit fixed)

When CPU is busy, the above sequential case fails occasionally, expand the timeout value to fix it. PR-URL: #16314 Fixes: #16310 Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>

When CPU is busy, the above sequential case fails occasionally, expand the timeout value to fix it. PR-URL: nodejs/node#16314 Fixes: nodejs/node#16310 Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>

When CPU is busy, the above sequential case fails occasionally, expand the timeout value to fix it. PR-URL: #16314 Fixes: #16310 Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>

When CPU is busy, the above sequential case fails occasionally, expand the timeout value to fix it. PR-URL: nodejs/node#16314 Fixes: nodejs/node#16310 Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>

nodejs-github-bot added the test Issues and PRs related to the tests. label Oct 19, 2017

gireeshpunathil approved these changes Oct 19, 2017

View reviewed changes

lpinca reviewed Oct 19, 2017

View reviewed changes

mscdex added the timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. label Oct 19, 2017

zhangzifa mentioned this pull request Oct 20, 2017

Flaky test-timers-block-eventloop #16310

Closed

test: update test-timers-block-eventloop.js

41f1e25

When CPU is busy, the above sequential case fails occasionally, expand the timeout value to fix it. Fixes: nodejs#16310

zhangzifa force-pushed the fix_issue_16310 branch from ea1d27c to 41f1e25 Compare October 20, 2017 11:42

gibfahn reviewed Oct 20, 2017

View reviewed changes

gibfahn approved these changes Oct 20, 2017

View reviewed changes

lpinca reviewed Oct 20, 2017

View reviewed changes

jasnell approved these changes Oct 20, 2017

View reviewed changes

refack approved these changes Oct 27, 2017

View reviewed changes

refack self-assigned this Oct 28, 2017

refack closed this Oct 29, 2017

jasnell mentioned this pull request Oct 30, 2017

Proposal: 2017-10-31, Version 9.0.0 (Current) #15135

Merged

gibfahn mentioned this pull request Oct 31, 2017

v8.9.0 proposal #16630

Merged

MylesBorins added the land-on-v6.x label Nov 16, 2017

MylesBorins mentioned this pull request Nov 21, 2017

v6.12.1 proposal #17180

Merged

refack removed their assignment Oct 12, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: update test-timers-block-eventloop.js #16314

test: update test-timers-block-eventloop.js #16314

zhangzifa commented Oct 19, 2017

lpinca Oct 19, 2017

Fishrock123 Oct 19, 2017

zhangzifa Oct 20, 2017

gibfahn Oct 20, 2017

lpinca Oct 20, 2017 •

edited

Loading

refack commented Oct 27, 2017 •

edited

Loading

joyeecheung commented Oct 28, 2017 •

edited

Loading

refack commented Oct 28, 2017

refack commented Oct 28, 2017

refack commented Oct 29, 2017

test: update test-timers-block-eventloop.js #16314

test: update test-timers-block-eventloop.js #16314

Conversation

zhangzifa commented Oct 19, 2017

Checklist

Affected core subsystem(s)

lpinca Oct 19, 2017

Choose a reason for hiding this comment

Fishrock123 Oct 19, 2017

Choose a reason for hiding this comment

zhangzifa Oct 20, 2017

Choose a reason for hiding this comment

gibfahn Oct 20, 2017

Choose a reason for hiding this comment

lpinca Oct 20, 2017 • edited Loading

Choose a reason for hiding this comment

refack commented Oct 27, 2017 • edited Loading

joyeecheung commented Oct 28, 2017 • edited Loading

refack commented Oct 28, 2017

refack commented Oct 28, 2017

refack commented Oct 29, 2017

lpinca Oct 20, 2017 •

edited

Loading

refack commented Oct 27, 2017 •

edited

Loading

joyeecheung commented Oct 28, 2017 •

edited

Loading