test: fix flaky test-http2-session-timeout #15338

apapirovski · 2017-09-11T11:13:56Z

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 · 2017-09-11T11:19:10Z

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 · 2017-09-12T08:48:55Z

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

tniessen

SGTM if CI passes.

apapirovski · 2017-09-12T12:00:59Z

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

mcollina · 2017-09-13T06:55:18Z

~~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 · 2017-09-13T11:10:23Z

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 · 2017-09-13T11:24:50Z

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 · 2017-09-13T19:07:02Z

(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 · 2017-09-13T19:09:56Z

Stress tests again:

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

apapirovski · 2017-09-13T19:10:10Z

Well, that explains that. Thanks @Trott

Trott · 2017-09-14T03:57:25Z

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 · 2017-09-14T03:57:44Z

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

Trott · 2017-09-14T04:00:29Z

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 · 2017-09-14T05:57:34Z

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 · 2017-09-14T10:34:27Z

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 · 2017-09-14T10:55:12Z

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

mcollina · 2017-09-14T12:00:26Z

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

apapirovski · 2017-09-14T12:18:42Z

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 · 2017-09-14T12:27:20Z

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

apapirovski · 2017-09-14T12:29:41Z

@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 · 2017-09-14T22:07:18Z

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 · 2017-09-14T22:10:32Z

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

Trott · 2017-09-14T22:42:55Z

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 · 2017-09-15T01:44:17Z

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 · 2017-09-15T16:27:27Z

No worries at all @apapirovski !

apapirovski · 2017-09-16T12:26:42Z

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 · 2017-09-16T12:44:06Z

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 · 2017-09-16T12:44:37Z

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

apapirovski · 2017-09-16T12:48:22Z

@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?

Increase server timeout, reduce frequency of calls and unbind timeout after runs are done in order to avoid race conditions. Temporarily moved to sequential. Fixes: nodejs#15326

Trott · 2017-09-16T13:01:04Z

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 · 2017-09-16T14:39:02Z

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

Trott · 2017-09-17T01:59:10Z

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

mcollina · 2017-09-18T10:50:11Z

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

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

apapirovski · 2017-09-18T11:56:54Z

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

mcollina

LGTM

mcollina · 2017-09-18T14:14:20Z

Landed as de51717.

Increase server timeout, reduce frequency of calls and unbind timeout after runs are done in order to avoid race conditions. Temporarily moved to sequential. Fixes: #15326 PR-URL: #15338 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de>

Increase server timeout, reduce frequency of calls and unbind timeout after runs are done in order to avoid race conditions. Temporarily moved to sequential. Fixes: nodejs/node#15326 PR-URL: nodejs/node#15338 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de>

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

apapirovski force-pushed the patch-fix-test-http2-session-timeout branch from a6b0f34 to c8e3767 Compare September 11, 2017 11:21

mscdex added the http2 Issues or PRs related to the http2 subsystem. label Sep 11, 2017

mcollina mentioned this pull request Sep 12, 2017

test: fix flaky test-http2-session-timeout #15328

Closed

2 tasks

tniessen approved these changes Sep 12, 2017

View reviewed changes

jasnell approved these changes Sep 12, 2017

View reviewed changes

hiroppy approved these changes Sep 13, 2017

View reviewed changes

apapirovski force-pushed the patch-fix-test-http2-session-timeout branch from c8e3767 to 3bd0870 Compare September 13, 2017 11:24

apapirovski force-pushed the patch-fix-test-http2-session-timeout branch from 3bd0870 to c47516c Compare September 14, 2017 10:53

apapirovski force-pushed the patch-fix-test-http2-session-timeout branch from c47516c to 65e5a0b Compare September 14, 2017 12:26

apapirovski force-pushed the patch-fix-test-http2-session-timeout branch from 65e5a0b to 3103f89 Compare September 14, 2017 12:29

apapirovski mentioned this pull request Sep 14, 2017

test-http2-session-timeout flaky on FreeBSD #15326

Closed

Trott mentioned this pull request Sep 16, 2017

test: move test-http2-session-timeout #15438

Closed

2 tasks

apapirovski force-pushed the patch-fix-test-http2-session-timeout branch from 3103f89 to 0c22399 Compare September 16, 2017 12:25

test: fix flaky test-http2-session-timeout

0113f2e

Increase server timeout, reduce frequency of calls and unbind timeout after runs are done in order to avoid race conditions. Temporarily moved to sequential. Fixes: nodejs#15326

apapirovski force-pushed the patch-fix-test-http2-session-timeout branch from 0c22399 to 0113f2e Compare September 16, 2017 14:36

mcollina approved these changes Sep 18, 2017

View reviewed changes

mcollina closed this Sep 18, 2017

apapirovski deleted the patch-fix-test-http2-session-timeout branch September 19, 2017 14:00

jasnell mentioned this pull request Sep 20, 2017

Node.js 8.6.0 Release Proposal #15509

Merged

MylesBorins added the dont-land-on-v6.x label Oct 17, 2017

gibfahn mentioned this pull request Dec 19, 2017

Investigate flaky parallel/test-listen-fd-ebadf #17762

Closed

test: fix flaky test-http2-session-timeout #15338

test: fix flaky test-http2-session-timeout #15338

Conversation

apapirovski commented Sep 11, 2017 • edited Loading

Checklist

Affected core subsystem(s)

apapirovski commented Sep 11, 2017 • edited Loading

mcollina commented Sep 12, 2017

tniessen left a comment

Choose a reason for hiding this comment

apapirovski commented Sep 12, 2017

mcollina commented Sep 13, 2017 • edited Loading

mcollina commented Sep 13, 2017

apapirovski commented Sep 13, 2017

Trott commented Sep 13, 2017

Trott commented Sep 13, 2017

apapirovski commented Sep 13, 2017

Trott commented Sep 14, 2017

Trott commented Sep 14, 2017

Trott commented Sep 14, 2017

Trott commented Sep 14, 2017 • edited Loading

apapirovski commented Sep 14, 2017 • edited Loading

apapirovski commented Sep 14, 2017

mcollina commented Sep 14, 2017

apapirovski commented Sep 14, 2017 • edited Loading

apapirovski commented Sep 14, 2017 • edited Loading

apapirovski commented Sep 14, 2017 • edited Loading

Trott commented Sep 14, 2017 • edited Loading

Trott commented Sep 14, 2017

Trott commented Sep 14, 2017

apapirovski commented Sep 15, 2017

jasnell commented Sep 15, 2017

apapirovski commented Sep 16, 2017

Trott commented Sep 16, 2017

Trott commented Sep 16, 2017

apapirovski commented Sep 16, 2017 • edited Loading

Trott commented Sep 16, 2017

apapirovski commented Sep 16, 2017 • edited Loading

Trott commented Sep 17, 2017

mcollina commented Sep 18, 2017

apapirovski commented Sep 18, 2017

mcollina left a comment

Choose a reason for hiding this comment

mcollina commented Sep 18, 2017

apapirovski commented Sep 11, 2017 •

edited

Loading

apapirovski commented Sep 11, 2017 •

edited

Loading

mcollina commented Sep 13, 2017 •

edited

Loading

Trott commented Sep 14, 2017 •

edited

Loading

apapirovski commented Sep 14, 2017 •

edited

Loading

apapirovski commented Sep 14, 2017 •

edited

Loading

apapirovski commented Sep 14, 2017 •

edited

Loading

apapirovski commented Sep 14, 2017 •

edited

Loading

Trott commented Sep 14, 2017 •

edited

Loading

apapirovski commented Sep 16, 2017 •

edited

Loading

apapirovski commented Sep 16, 2017 •

edited

Loading