parallel/test-http-set-timeout failing occasionally

[BethGriggs]

• Version: 6.8.0 & 6.9.1
• Platform: ppc64le

parallel/test-http-set-timeout

I have seen this test fail up to 10 times in 5000 runs on ppc64le (Ubuntu 14.04) with the error
'throw new Error('Timeout was not successful');'.

Also, the console.log on line 8 states 'setting 1 second timeout', despite the timeout on line 9 being set to 500ms.

[gibfahn]

@Trott the timer on test-http-set-timeout.js#L21 seems unnecessary (I'm pretty sure the timer being tested is the one on Line 9). Is there any reason not to just wait for the test-runner timeout?

[Trott]

@gibfahn It looks like that to me. There's definitely a faction (or maybe the entirety? certainly includes me) of @nodejs/testing that is all for removing timers like this.

FWIW: The argument against removing it is that when the test fails via command line like ./node test/parallel/test-http-settimeout.js, it will hang forever and not give any indication as to what the problem is. I've thought about creating something in common.js that can be called in tests like this that detects if we are in CI or not and will set a timer if and only if we are not in CI. But that introduces its own complexity and I"m not sure it's worth it.

But yeah, +1 from me on removing that timer if it helps this situation and doesn't reduce the validity of the test.

[BethGriggs]

I have managed to reproduce this error on darwin-x64 v6.9.1 (in 3000 runs I saw 3 failures).

[BethGriggs]

Is it possible that the req.connection.on('timeout') event listener (test-http-set-timeout.js#L11) is occasionally registered after the 500ms req.connection.setTimeout has already passed?

[Trott]

Just to be sure, if you can reproduce the error reasonably reliably in the existing test (like, there's always at least one failure in 5K runs or something), you could try moving lines 9 and 10 to the end of the function and see if that fixes it. Even if there's no way that should be happening, it's not a bad idea to confirm...