http2: verify that flooding error happens

[jasnell]

verify protections against ping and settings flooding

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)

http2

[jasnell]

ping @nodejs/http2

[mcollina]

LGTM

[jasnell]

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

[jasnell]

Seeing some flakiness across platforms. The reason appears to be:

1. Some platforms respond to pings/settings at different rates therefore making the exact timing of a ping/settings flood error a bit difficult to predict. Hopefully writing them at a faster pace will ensure we hit it on all platforms.

2. There is some variance on windows in the timing of the ECONNRESET error that I'm still tracking down.

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

[jasnell]

Still flaky... trying again: https://ci.nodejs.org/job/node-test-pull-request/12430/

[jasnell]

Trying once again... https://ci.nodejs.org/job/node-test-pull-request/12431/

It's entirely possible that we won't be able to reliable test for ping and settings flood generically on all systems. In which case, I will move the tests out of parallel and have to mark them flaky

[jasnell]

So close... but still seeing flakiness on the flood tests on Windows. Moved to sequential and marked flaky. Those are going to be generally unreliable as a rule due to the nature of the error and the code that triggers it. We need the coverage either way but will have to investigate some way of making them less flaky later.

[jasnell]

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

[jasnell]

@mcollina ... can you take one more look at this.

[mcollina]

I would prefer this to be a proper a common.mustCall() with an error check.

[jasnell]

Whether or not an error happens here is dependent entirely on operating system. It's quite flaky.

[mcollina]

I fear this might be time sensitive. Can we avoid it anyhow? Maybe add a comment here

[jasnell]

We can with a bit of dancing between the client and server. Will take another pass.

[jasnell]

actually, no dancing required. pretty straight forward :-)

[mcollina]

can you make this a proper function?

[jasnell]

In which sense? client.on('error', function(){}) ? I can, but I'm curious about why it matters in this case.

[mcollina]

I mean, handling/checking the error, you answered on another file. Why don't you put a note about the fact that the error can or cannot happen depending on the OS?

[jasnell]

Already done :-)

[mcollina]

can you make this a proper function?

[jasnell]

New CI before landing: https://ci.nodejs.org/job/node-test-pull-request/12444/

[jasnell]

New CI: https://ci.nodejs.org/job/node-test-pull-request/12445/
Had to adjust a couple of the tests because of another PR that landed

[jasnell]

Once more... with feeling https://ci.nodejs.org/job/node-test-pull-request/12446/

[jasnell]

CI is good

[jasnell]

Landed in 3303987

[Trott]

Can we revert this until the test doesn't fail all the time on Windows?

[Trott]

(If the problem is Windows and not the code here, then the solution is probably not to mark the test as flaky but to not bother running the test on Windows at all. Skip it instead. There is no point in running a test if the results are meaningless.)

[jasnell]

I'm good with skipping on windows for now. Just want to make sure we can revisit this.

[Trott]

Maybe skip on Windows and open an issue in the issue tracker about how it probably could be made to work on Windows and put as much info there as you can?

[jasnell]

I'll (1) open a PR that skips on windows, (2) add a known-issue test that skips everything but windows, and (3) open an issue that discusses it. The key thing to remember here is that the failure is not an actual failure, the code is working as it is supposed to on Windows, the test just does not have the ability to trigger the error condition on Windows.