http2: verify that flooding error happens #17969

jasnell · 2018-01-03T21:37:33Z

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 · 2018-01-03T21:37:48Z

ping @nodejs/http2

mcollina

LGTM

jasnell · 2018-01-05T20:39:25Z

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

jasnell · 2018-01-05T21:44:20Z

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

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.
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 · 2018-01-05T22:14:06Z

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

jasnell · 2018-01-05T23:11:33Z

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 · 2018-01-06T00:02:35Z

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 · 2018-01-06T00:02:45Z

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

jasnell · 2018-01-06T00:47:38Z

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

mcollina · 2018-01-08T16:59:23Z

test/parallel/test-http2-ping-unsolicited-ack.js

+    });
+  }));
+
+  client.on('error', () => {});


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

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

mcollina · 2018-01-08T17:00:45Z

test/parallel/test-http2-settings-unsolicited-ack.js

+      client.write(kSettings.data, () => {
+        // Give time to receive the servers settings frame
+        // so that the first ack below is actually expected.
+        setImmediate(() => {


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

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

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

mcollina · 2018-01-08T17:01:10Z

test/sequential/test-http2-ping-flood.js

+  }));
+
+  // We don't care if an error is emitted here, but don't crash
+  client.on('error', () => {});


can you make this a proper function?

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

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?

Already done :-)

mcollina · 2018-01-08T17:01:19Z

test/sequential/test-http2-settings-flood.js

+  }));
+
+  // We don't care if an error is emitted here, but don't crash
+  client.on('error', () => {});


can you make this a proper function?

jasnell · 2018-01-08T18:31:30Z

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

jasnell · 2018-01-08T19:07:17Z

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 · 2018-01-08T20:23:01Z

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

* verify protections against ping and settings flooding * Strictly handle and verify handling of unsolicited ping and settings frame acks.

jasnell · 2018-01-08T21:09:20Z

CI is good

* verify protections against ping and settings flooding * Strictly handle and verify handling of unsolicited ping and settings frame acks. PR-URL: #17969 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

jasnell · 2018-01-08T21:12:20Z

Landed in 3303987

* verify protections against ping and settings flooding * Strictly handle and verify handling of unsolicited ping and settings frame acks. PR-URL: nodejs#17969 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

* verify protections against ping and settings flooding * Strictly handle and verify handling of unsolicited ping and settings frame acks. Backport-PR-URL: #18050 PR-URL: #17969 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

Trott · 2018-01-10T16:58:59Z

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

Trott · 2018-01-10T17:01:50Z

(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 · 2018-01-10T17:02:51Z

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

Trott · 2018-01-10T19:56:20Z

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 · 2018-01-10T20:09:53Z

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.

* verify protections against ping and settings flooding * Strictly handle and verify handling of unsolicited ping and settings frame acks. Backport-PR-URL: nodejs#18050 PR-URL: nodejs#17969 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

* verify protections against ping and settings flooding * Strictly handle and verify handling of unsolicited ping and settings frame acks. Backport-PR-URL: #18050 Backport-PR-URL: #20456 PR-URL: #17969 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

jasnell added http2 Issues or PRs related to the http2 subsystem. test Issues and PRs related to the tests. labels Jan 3, 2018

addaleax approved these changes Jan 3, 2018

View reviewed changes

apapirovski approved these changes Jan 3, 2018

View reviewed changes

jasnell force-pushed the http2-verifyflooding branch from 3aef309 to 0667401 Compare January 3, 2018 22:40

mcollina approved these changes Jan 4, 2018

View reviewed changes

jasnell force-pushed the http2-verifyflooding branch from 0667401 to a881ade Compare January 5, 2018 21:14

jasnell force-pushed the http2-verifyflooding branch 2 times, most recently from 77e97cc to 4ca203c Compare January 6, 2018 00:46

mcollina reviewed Jan 8, 2018

View reviewed changes

jasnell force-pushed the http2-verifyflooding branch from ad66c46 to de9a99a Compare January 8, 2018 19:06

http2: verify flood error and unsolicited frames

c8287db

* verify protections against ping and settings flooding * Strictly handle and verify handling of unsolicited ping and settings frame acks.

jasnell force-pushed the http2-verifyflooding branch from 3bd5cb0 to c8287db Compare January 8, 2018 21:09

jasnell closed this Jan 8, 2018

MylesBorins mentioned this pull request Jan 10, 2018

v9.4.0 proposal #18069

Merged

MylesBorins added the dont-land-on-v6.x label Jan 19, 2018

MylesBorins mentioned this pull request May 2, 2018

v8.11.2 proposal #20478

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

http2: verify that flooding error happens #17969

http2: verify that flooding error happens #17969

jasnell commented Jan 3, 2018

jasnell commented Jan 3, 2018

mcollina left a comment

jasnell commented Jan 5, 2018

jasnell commented Jan 5, 2018

jasnell commented Jan 5, 2018

jasnell commented Jan 5, 2018

jasnell commented Jan 6, 2018

jasnell commented Jan 6, 2018

jasnell commented Jan 6, 2018

mcollina Jan 8, 2018

jasnell Jan 8, 2018

mcollina Jan 8, 2018

jasnell Jan 8, 2018

jasnell Jan 8, 2018

mcollina Jan 8, 2018

jasnell Jan 8, 2018

mcollina Jan 8, 2018

jasnell Jan 8, 2018

mcollina Jan 8, 2018

jasnell commented Jan 8, 2018

jasnell commented Jan 8, 2018

jasnell commented Jan 8, 2018

jasnell commented Jan 8, 2018

jasnell commented Jan 8, 2018

Trott commented Jan 10, 2018

Trott commented Jan 10, 2018

jasnell commented Jan 10, 2018

Trott commented Jan 10, 2018

jasnell commented Jan 10, 2018

http2: verify that flooding error happens #17969

http2: verify that flooding error happens #17969

Conversation

jasnell commented Jan 3, 2018

Checklist

Affected core subsystem(s)

jasnell commented Jan 3, 2018

mcollina left a comment

Choose a reason for hiding this comment

jasnell commented Jan 5, 2018

jasnell commented Jan 5, 2018

jasnell commented Jan 5, 2018

jasnell commented Jan 5, 2018

jasnell commented Jan 6, 2018

jasnell commented Jan 6, 2018

jasnell commented Jan 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasnell commented Jan 8, 2018

jasnell commented Jan 8, 2018

jasnell commented Jan 8, 2018

jasnell commented Jan 8, 2018

jasnell commented Jan 8, 2018

Trott commented Jan 10, 2018

Trott commented Jan 10, 2018

jasnell commented Jan 10, 2018

Trott commented Jan 10, 2018

jasnell commented Jan 10, 2018