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

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

/cc @jasnell @addaleax

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

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().

👍

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.

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

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

@nodejs/http2 @nodejs/testing

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

Fixed the comments. PTAL @addaleax @Trott @jasnell

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.

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

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

oy! obvious and simple in hindsight.

a classic @mcollina fix :)

LGTM, especially with the fix :-)

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

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

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

@apapirovski I've added your commit on top.

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

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).

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/)

@jasnell @addaleax @trivikr PTAL

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>

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 3dc5404 Compare 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 5b55170 Compare April 6, 2018 22:05

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

mafintosh force-pushed the add-failing-http2-test-with-writes branch from 17630b1 to cf5a405 Compare 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 9cc9a9d Compare 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 outstanding review comments, and a CI started. label Apr 12, 2018

addaleax removed the author ready PRs that have at least one approval, no outstanding review comments, 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

mafintosh and 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 c3c1f3d Compare May 10, 2018 14:18

apapirovski force-pushed the add-failing-http2-test-with-writes branch 2 times, most recently from 56c994c to a402353 Compare 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 53eb584 Compare 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

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

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

Uh oh!

Uh oh!

http2: add failing writes with destroy test - #19852

http2: add failing writes with destroy test#19852mafintosh wants to merge 4 commits intonodejs:masternodejs/node:masterfrom mafintosh:add-failing-http2-test-with-writesmafintosh/io.js:add-failing-http2-test-with-writesCopy head branch name to clipboard

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

http2: add failing writes with destroy test#19852
mafintosh wants to merge 4 commits into
nodejs:masterfrom
mafintosh:add-failing-http2-test-with-writes

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