http2: add failing writes with destroy test by mafintosh · Pull Request #19852 · nodejs/node

mafintosh · 2018-04-06T15:54:02Z

Checklist

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

Simplified the failing http2 test from #19828 into this test case. Basically when doing lots of writes and then destroying the request, the process will hang instead of exiting normally.

Fixes#20630

mafintosh · 2018-04-06T15:54:27Z

/cc @jasnell @addaleax

addaleax

This would need to be rebased due to #19854, sorry :/

addaleax · 2018-04-06T16:58:10Z

I think we've been removing these kinds of checks to avoid timeout-related flakiness in tests -- the test runner enforces its own timeout anyway. (/cc @Trott)
If you want to have actual timing guarantees, you can try to remove the dependency on networking and use something like test/parallel/test-http2-generic-streams.js does.

Other things to be aware of, just sorta FYI, you don't need to do anything with these at this time unless you think you should:
Tests with timers like this can become unreliable if the test is competing with other tests for resources. This unreliability is introduced surprisingly often on a few platforms in CI (particularly FreeBSD, I think because test.py decides it has 48 processors or something but they're not "real" processors--never really dug into it TBH, but you get the idea). To avoid the problem, the test can be put in sequential rather than parallel.
If you think 10 seconds is plenty for most things, but that maybe a Raspberry Pi could use a little more time than that, use common.platformTimeout().

addaleax · 2018-04-06T16:58:39Z

I think this could be common.mustCallAtLeast()'ed, to make sure it's run?

Since it's a once handler, it could be common.mustCall().
If the test will fail without the handler running, then I'm OK omitting common.mustCall(). And I'm OK including it. Either way...

fixed. added a mustCall, since it's a once.

Trott · 2018-04-06T22:05:31Z

This would need to be rebased due to #19854, sorry :/

I did the rebase, so no need to worry about that anymore.

Trott · 2018-04-06T22:06:01Z

@nodejs/http2 @nodejs/testing

BridgeAR · 2018-04-08T17:16:36Z

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

mafintosh · 2018-04-09T10:00:29Z

Fixed the comments. PTAL @addaleax @Trott @jasnell

mcollina · 2018-04-11T14:55:44Z

This test is failing and it is showing a bug :/.

I've run why-is-node-running with it, and we just have:

There are 7 handle(s) keeping the process running
# Timeout
/Users/matteo/Repositories/node/test/parallel/test-http2-many-writes-and-destroy.js:35 - setTimeout(function () {
# TCPWRAP
/Users/matteo/Repositories/node/test/parallel/test-http2-many-writes-and-destroy.js:17 - const client = http2.connect(url);
# HTTP2SESSION
(unknown stack trace)
# HTTP2SETTINGS
(unknown stack trace)
# Timeout
(unknown stack trace)
# HTTP2SESSION
(unknown stack trace)
# HTTP2STREAM
(unknown stack trace)

I have a possible fix coming, pushing directly to this PR.

mafintosh · 2018-04-11T15:43:06Z

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

mafintosh · 2018-04-11T15:43:38Z

🎉 @mcollina added a fix for my failing test case \o/

jasnell · 2018-04-11T15:53:36Z

 if (!socket.destroyed) {
 if (!error) {
- setImmediate(socket.end.bind(socket));
+ setImmediate(socket.destroy.bind(socket));


oy! obvious and simple in hindsight.

a classic @mcollina fix :)

jasnell · 2018-04-11T15:54:06Z

LGTM, especially with the fix :-)

BridgeAR · 2018-04-13T18:23:57Z

Rerunning the CI because there were a couple of failures https://ci.nodejs.org/job/node-test-pull-request/14265/

addaleax · 2018-04-14T16:07:58Z

@mafintosh Can you make sure that the CI failures are unrelated? In particular, at a quick glance https://ci.nodejs.org/job/node-test-commit-osx/17798/nodes=osx1010/console might not be…

apapirovski · 2018-04-24T07:04:33Z

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

mcollina · 2018-05-10T12:42:35Z

@apapirovski I've added your commit on top.

mcollina · 2018-05-10T12:43:14Z

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

apapirovski · 2018-05-10T14:20:41Z

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

I added some other fixes for the new changes. I also removed the changes to the http2 pipe test because if that one's getting an ECONNRESET then we shouldn't swallow it. That would indicate the fix here isn't fully correct (e.g. it's possible for us to be still writing meaningful data when the socket is destroyed).

apapirovski · 2018-05-11T04:46:35Z

Ok, the CI is green FWIW. If someone wants to give this a few more looks then we could go ahead and finally land this.

(Another CI just to be safe: https://ci.nodejs.org/job/node-test-pull-request/14805/)

mcollina · 2018-05-11T22:01:33Z

@jasnell @addaleax @trivikr PTAL

jasnell · 2018-05-12T23:05:44Z

Still LGTM

Fix a bug where the socket wasn't being correctly destroyed and adjust existing tests, as well as add additional tests. PR-URL: #19852 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Co-authored-by: Matteo Collina <matteo.collina@gmail.com>

apapirovski · 2018-05-13T17:50:57Z

Landed in b60b183

Fix a bug where the socket wasn't being correctly destroyed and adjust existing tests, as well as add additional tests. PR-URL: #19852 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Co-authored-by: Matteo Collina <matteo.collina@gmail.com>

nodejs-github-bot added the test Issues and PRs related to the tests. label Apr 6, 2018

mafintosh added the http2 Issues or PRs related to the http2 subsystem. label Apr 6, 2018

addaleax force-pushed the master branch from 4ae53af to 3dc5404Compare April 6, 2018 16:54

addaleax approved these changes Apr 6, 2018
View reviewed changes

Trott force-pushed the add-failing-http2-test-with-writes branch from be13bfe to 5b55170Compare April 6, 2018 22:05

mafintosh force-pushed the add-failing-http2-test-with-writes branch from 5b55170 to 17630b1Compare April 9, 2018 09:58

mafintosh force-pushed the add-failing-http2-test-with-writes branch from 17630b1 to cf5a405Compare April 9, 2018 11:26

jasnell approved these changes Apr 9, 2018
View reviewed changes

mcollina force-pushed the add-failing-http2-test-with-writes branch from cf5a405 to 9cc9a9dCompare April 11, 2018 15:39

jasnell reviewed Apr 11, 2018
View reviewed changes

trivikr approved these changes Apr 11, 2018
View reviewed changes

BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 12, 2018

addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 14, 2018

mcollina mentioned this pull request Apr 16, 2018
difference between http2 Compatibility API and http #20060
Closed

mcollina mentioned this pull request Apr 24, 2018
streamError never emitted #20211
Closed

apapirovski mentioned this pull request May 10, 2018
http2: server.close not behaving like http1 or net #20630
Closed

mafintoshand others added 2 commits May 10, 2018 08:54

http2: add failing writes with destroy test
2df7396

http2: destroy the underlining socket on destroy
e7b7035

apapirovski force-pushed the add-failing-http2-test-with-writes branch from e3f9922 to c3c1f3dCompare May 10, 2018 14:18

apapirovski force-pushed the add-failing-http2-test-with-writes branch 2 times, most recently from 56c994c to a402353Compare May 10, 2018 14:44

test: fix flaky http2 tests
53eb584

apapirovski force-pushed the add-failing-http2-test-with-writes branch from a402353 to 53eb584Compare May 10, 2018 14:55

trivikr approved these changes May 12, 2018
View reviewed changes

apapirovski closed this May 13, 2018

addaleax mentioned this pull request May 14, 2018
v10.2.0 proposal #20724
Merged

addaleax added the land-on-v8.x label Jun 29, 2018

MylesBorins mentioned this pull request Jul 9, 2018
v8.12.0 proposal #21593
Merged

Trott mentioned this pull request Nov 25, 2018
meta: patch branches #24467
Closed

lundibundi mentioned this pull request Dec 8, 2019
http2: fix graceful session close #30854
Closed
4 tasks

Uh oh!

Uh oh!

Conversation

mafintosh commented Apr 6, 2018• edited by mcollina Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

mafintosh commented Apr 6, 2018

Uh oh!

addaleax left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Trott commented Apr 6, 2018

Uh oh!

Trott commented Apr 6, 2018

Uh oh!

BridgeAR commented Apr 8, 2018

Uh oh!

mafintosh commented Apr 9, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcollina commented Apr 11, 2018

Uh oh!

mafintosh commented Apr 11, 2018

Uh oh!

mafintosh commented Apr 11, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasnell commented Apr 11, 2018

Uh oh!

BridgeAR commented Apr 13, 2018

Uh oh!

addaleax commented Apr 14, 2018

Uh oh!

apapirovski commented Apr 24, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcollina commented May 10, 2018

Uh oh!

mcollina commented May 10, 2018

Uh oh!

apapirovski commented May 10, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

apapirovski commented May 11, 2018

Uh oh!

mcollina commented May 11, 2018

Uh oh!

jasnell commented May 12, 2018

Uh oh!

apapirovski commented May 13, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

mafintosh commented Apr 6, 2018•
edited by mcollina
Loading

mafintosh commented Apr 9, 2018•
edited
Loading

apapirovski commented Apr 24, 2018•
edited
Loading

apapirovski commented May 10, 2018•
edited
Loading