Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Forget closed http2 streams #23134

Closed
wants to merge 4 commits into from
Closed

Forget closed http2 streams #23134

wants to merge 4 commits into from

Conversation

davedoesdev
Copy link
Contributor

@davedoesdev davedoesdev commented Sep 27, 2018 •
edited
Loading

Fixes: #23116

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. dont-land-on-v6.x http2 Issues or PRs related to the http2 subsystem. labels Sep 27, 2018
@addaleax
Copy link
Member

@nodejs/http2

@jasnell
Copy link
Member

jasnell commented Sep 28, 2018

Lgtm with green ci

@addaleax
Copy link
Member

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

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 28, 2018
@davedoesdev
Copy link
Contributor Author

I'm afraid I don't understand the test results.
I can see a test timeout for this change, which doesn't occur when I run it locally.

@davedoesdev
Copy link
Contributor Author

davedoesdev commented Sep 28, 2018 •
edited
Loading

I can make the test run quicker by decreasing maxSessionMemory and thus the number of iterations

mcollina
mcollina approved these changes Sep 28, 2018
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jasnell
Copy link
Member

jasnell commented Sep 28, 2018

The test likely should go into test/sequential or maybe even test/pummel

@addaleax
Copy link
Member

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

@davedoesdev If a tests takes longer than 20s or so, you might want to consider moving it to test/pummel/. But if we can avoid that, that would be ideal, because those tests aren’t run as part of regular CI runs.

@davedoesdev
Copy link
Contributor Author

The revised test should only take a few seconds.

The new CI test failure on Linux is test.parallel/test-gc-http-client-timeout.
The failure on Windows is test-net-connect-options-port

@danbev
Copy link
Contributor

danbev commented Oct 2, 2018

Re-run of CI: https://ci.nodejs.org/job/node-test-pull-request/17576/

@davedoesdev
Forget closed http2 streams
f6642cf 
Fixes: #23116
@davedoesdev
Add comment explaining call to nghttp2_option_set_no_closed_streams
7507015
@davedoesdev
Issue #23116: Decrease test run time
6467dc1 
Decreasing maxSessionMemory lets us decrease number of iteractions
before the exception occurs without the fix.
@davedoesdev
Copy link
Contributor Author

Those tests are failing ontest-gc-net-timeout.

I notice that other PRs (e.g. https://ci.nodejs.org/job/node-test-pull-request/17567/) are also failing.

Are the CI tests usually 100% pass?

lpinca
lpinca approved these changes Oct 2, 2018
View reviewed changes
test/parallel/test-http2-forget-closed-streams.js Outdated
endStream: true
});
}));
stream.pipe(new Writable({
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: can't we use stream.resume() instead of piping to a dummy writable?

@lpinca
Copy link
Member

lpinca commented Oct 2, 2018 •
edited
Loading

Are the CI tests usually 100% pass?

No, there are some flaky tests.

@joyeecheung joyeecheung mentioned this pull request Oct 2, 2018
Closed
@danbev
Copy link
Contributor

danbev commented Oct 3, 2018 •
edited
Loading

Re-run of failing node-test-commit-linux ✔️
Re-run of failing node-test-commit-smartos ✔️

@danbev
Copy link
Contributor

danbev commented Oct 3, 2018

Landed in e8121d5.

@danbev danbev closed this Oct 3, 2018
danbev pushed a commit that referenced this pull request Oct 3, 2018
@davedoesdev @danbev
http2: set nghttp2_option_set_no_closed_streams
e8121d5 
PR-URL: #23134
Fixes: #23116
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Oct 4, 2018
@davedoesdev @targos
http2: set nghttp2_option_set_no_closed_streams
001881f 
PR-URL: #23134
Fixes: #23116
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@targos targos mentioned this pull request Oct 7, 2018
Merged
@beevelop beevelop mentioned this pull request Oct 10, 2018
Closed
This was referenced Oct 10, 2018
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
jasnell pushed a commit that referenced this pull request Oct 17, 2018
@davedoesdev @jasnell
http2: set nghttp2_option_set_no_closed_streams
bdf1871 
PR-URL: #23134
Fixes: #23116
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@addaleax addaleax mentioned this pull request Oct 20, 2018
Closed
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lpinca lpinca lpinca approved these changes

@jasnell jasnell jasnell approved these changes

@mcollina mcollina mcollina approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. http2 Issues or PRs related to the http2 subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

HTTP/2 POST loop fails with ENHANCE_YOUR_CALM after 40701 iterations
7 participants
@davedoesdev @addaleax @jasnell @danbev @lpinca @mcollina @nodejs-github-bot