test: fix flaky test-http2-session-timeout

[apapirovski]

This is an alternative solution to #15328. Increases server timeout, reduces frequency of calls and unbinds timeout after runs are done in order to avoid race conditions.

cc @Trott@mcollina@tniessen

Checklist

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

Affected core subsystem(s)

test

[apapirovski]

I was able to run tools/test.py -j 96 --repeat 192 test/parallel/test-http2-session-timeout.js with no failures (repeatedly) so this should be safe.

[mcollina]

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

[tniessen]

SGTM if CI passes.

[apapirovski]

We should be able to run a CI stress test now that @TimothyGu's patch landed. Could someone pull the trigger, please?

[mcollina]

stress test: https://ci.nodejs.org/job/node-stress-single-test/1410/ and https://ci.nodejs.org/job/node-stress-single-test/1411/ (for both FreeBSDs).

https://ci.nodejs.org/job/node-stress-single-test/1412/
https://ci.nodejs.org/job/node-stress-single-test/1413/

[mcollina]

Both of them failed all times, so maybe I got something wrong in kicking off the job. Can someone else have a look and start it again?

[apapirovski]

It behaved as if the patch to the test runner didn't land. Not sure why that would be.

(I've rebased this just in case but I can't imagine it's running my version of the test runner... is it?)

[Trott]

(I've rebased this just in case but I can't imagine it's running my version of the test runner... is it?)

I think it is (or was). It was using ref refs/pull/15338/head and doesn't appear to do a rebase.

[Trott]

Stress tests again:

https://ci.nodejs.org/job/node-stress-single-test/1415/
https://ci.nodejs.org/job/node-stress-single-test/1416/

[apapirovski]

Well, that explains that. Thanks @Trott

[Trott]

Whoops, those stress tests aren't really valid because they aren't running things concurrently. (Glad their green, though.)

I'll kick off some more in a minute.

[Trott]

Oh, this should include Fixes: #15326 as metadata. This is a note to whoever lands this. :-D

[Trott]

Stress test on freebsd10-64 where it has been observed to fail:

This PR (should be green): https://ci.nodejs.org/job/node-stress-single-test/1417/

master (should be red): https://ci.nodejs.org/job/node-stress-single-test/1418/

[Trott]

The stress test still fails with these changes. (EDIT: I stopped the one for this PR after 76 failures in 76 runs.)

For comparison, here's a stress test on #15328: https://ci.nodejs.org/job/node-stress-single-test/1419/nodes=freebsd10-64/console EDIT: Argh, looks like I need to rebase to run a single test as well. Did that now, so let's try again: https://ci.nodejs.org/job/node-stress-single-test/1420/nodes=freebsd10-64/console

If that stress test passes but you still want the overall changes in this PR, hopefully the solution is to simply change the timeout values from 500 and 50 to 1200 and 10. Not a great solution as there's still a race condition, but one that (probably) won't be triggered on our CI or in most developer test runs.

If that stress test fails too, then it's time to look into things some more...

[apapirovski]

The problem with comparing this to the other PR is that it doesn't actually test the timeout condition, as per my comment there. It only runs 40 requests 10ms (400ms total) apart and the timeout is 1200ms.

I'll see what can be done to fix this PR.

[apapirovski]

Ok, changed up some more stuff. Should be able to handle concurrency better now. Can we run this again? Thanks so much!

[mcollina]

Stress test: https://ci.nodejs.org/job/node-stress-single-test/1421/

[apapirovski]

Is there a margin of failure we consider acceptable for stress tests? Looks like it's failing about 15-20% of the time now.

I'm guessing if I bump up the timeout to something like 5000 and the calls to, say, 800 then it'll probably be fine on that system but it makes the test unnecessarily long on all platforms. Not sure if that's a big deal or not.

[apapirovski]

New version is up, 5000 & 800 now. If this doesn't do it then I don't know...

[apapirovski]

@mcollina I just checked the CI and the commit it's testing is the one from the other PR. I reverted my changes since I don't think they got tested.

[Trott]

So here are the important parameters to use to kick of a CI stress test for this PR:

• GIT_REMOTE_REF should be refs/pull/15338/head
• RUN_TESTS should be -j 96 --repeat 192 parallel/test-http2-session-timeout. If you don't include the -j and --repeat parameters, then the stress test runs one test at a time over and over, as if it was in sequential. This is fine for tests that are flaky for reasons other than concurrency, but this one is (like most timer-based race conditions) greatly impacted by how many other tests are running. 96 is way more than CI will typically run, but this is a way to make sure the problem is really solved. :-D
• RUN_TIMES leave at 100 because each one of those will run the test 192 times, so that means you're running the test 19,200 rimwa, which is plenty.
• Choose freebsd10-64 from the RUN_LABEL drop-down as that's where we've seen it fail in CI.

[Trott]

New CI: https://ci.nodejs.org/job/node-stress-single-test/1422/

[Trott]

Still failing. I think moving the test from test/parallel to test/sequential may be the way to go. Given the reliance on timers, I don't think there's any way to make this test not prone to race conditions.

[apapirovski]

A lot closer this time but yeah, still failing. Will think about it a bit and see if I can revise it in any way, if not we'll just have to move it to sequential. Sorry for the hassle, everyone.

[jasnell]

No worries at all @apapirovski !

[apapirovski]

Updated to move to sequential for now. Will revisit later potentially but all the http timeout tests are there so it might not be possible anyway.

[Trott]

Question that can be ignored if you wish in the interest of getting the fix landed quickly: Since it's in sequential, does it make sense to go back to a 200ms timeout or at least reduce it somewhat from 2000 to speed up the test?

[Trott]

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

[apapirovski]

@Trott Yeah, we can definitely reduce it. I'll put it at 200 for the server & 20 for the call. Did you want to run the CI on the updated version once I put it up or is that run above enough?

[Trott]

Did you want to run the CI on the updated version once I put it up

Yeah, let's run CI again after any changes.

[apapirovski]

Ok, timing updates are available to test now. Thanks @Trott!

[Trott]

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

[mcollina]

CI (again): https://ci.nodejs.org/job/node-test-pull-request/10118/

The failure seemed unrelated, but it would be better to check.

[apapirovski]

Failure looks unrelated, this is the test that failed parallel/test-listen-fd-ebadf.

[mcollina]

LGTM

[mcollina]

Landed as de51717.