test: test-timers-unrefd-interval-still-fires.js flaky on smartOS

[MylesBorins]

Seeing flaky results in LTS

https://ci.nodejs.org/job/node-test-commit-smartos/813/

# [FAIL] Interval fired 5/5 times.
# /home/iojs/build/workspace/node-test-commit-smartos/nodes/smartos14-64/test/parallel/test-timers-unrefd-interval-still-fires.js:14
#   throw new Error('Test timed out. keepOpen was not canceled.');
#   ^
# 
# Error: Test timed out. keepOpen was not canceled.
#     at null._onTimeout (/home/iojs/build/workspace/node-test-commit-smartos/nodes/smartos14-64/test/parallel/test-timers-unrefd-interval-still-fires.js:14:9)
#     at Timer.listOnTimeout (timers.js:92:15)

https://ci.nodejs.org/job/node-test-commit-smartos/810/

not ok 757 test-timers-unrefd-interval-still-fires.js
# [FAIL] Interval fired 4/5 times.
# /home/iojs/build/workspace/node-test-commit-smartos/nodes/smartos14-64/test/parallel/test-timers-unrefd-interval-still-fires.js:14
#   throw new Error('Test timed out. keepOpen was not canceled.');
#   ^
# 
# Error: Test timed out. keepOpen was not canceled.
#     at null._onTimeout (/home/iojs/build/workspace/node-test-commit-smartos/nodes/smartos14-64/test/parallel/test-timers-unrefd-interval-still-fires.js:14:9)
#     at Timer.listOnTimeout (timers.js:92:15)

Passes on job 811 and 812 though.

Running one more time to see if it fails again

https://ci.nodejs.org/job/node-test-commit-smartos/814/

[Trott]

Current version fails stress test: https://ci.nodejs.org/job/node-stress-single-test/307/nodes=smartos14-64/console

This would appear to be a fulfillment of the prophecy of @misterdjules at #3550 (comment).

I've refactored the test to not rely on an arbitrary timeout as it is not a performance benchmark but a functionality test designed to check for a very specific bug.

The refactored test passes the stress test: https://ci.nodejs.org/job/node-stress-single-test/311/nodes=smartos14-64/console

PR: #4561

[Fishrock123]

One of the aforementioned failures is particularly interesting:

# [FAIL] Interval fired 5/5 times.
# /home/iojs/build/workspace/node-test-commit-smartos/nodes/smartos14-64/test/parallel/test-timers-unrefd-interval-still-fires.js:14
#   throw new Error('Test timed out. keepOpen was not canceled.');
#   ^
# 
# Error: Test timed out. keepOpen was not canceled.
#     at null._onTimeout (/home/iojs/build/workspace/node-test-commit-smartos/nodes/smartos14-64/test/parallel/test-timers-unrefd-interval-still-fires.js:14:9)
#     at Timer.listOnTimeout (timers.js:92:15)

As far as I can tell, it isn't possible for this to happen. If it's 5/5 the timeout that throws that error will already be cancelled...

[Trott]

@Fishrock123 Perhaps a race condition caused by the clearTimeout(keepOpen) being in a setImmediate() rather than just called directly? I'm not sure that's possible, but since the impossible is happening, something is up.

[Fishrock123]

Oh hmmm.

@Trott I can't find where I suggested that to be added, I forget why that was. It should be removed, I think. (The setImmediate())

[Trott]

@Fishrock123 My first attempt at a fix for this was to just remove the setImmediate() (and convert arrow functions to ES5-compatible functions) and leave everything else: b954d83

It was still flaky when stress tested, though:
https://ci.nodejs.org/job/node-stress-single-test/308/nodes=smartos14-64/console

But it did get rid of the 5/5. The failures were all 4/5, 3/5...

The control stress test (against master) showed 5/5 failures: https://ci.nodejs.org/job/node-stress-single-test/307/nodes=smartos14-64/console

So yeah, it looks like the setImmediate() caused the 5/5 weirdness. (But fixing that did not fix the test flakiness.)

At least that mystery is solved...