FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Forget closed http2 streams by davedoesdev · Pull Request #23134 · nodejs/node · GitHub

/ node Public

Forget closed http2 streams - #23134

Closed
davedoesdev wants to merge 4 commits into
nodejs:masterfrom
davedoesdev:issue-23116-forget-closed-http2-streams
ConversationCommits (4)ChecksFiles changed
Closed

Forget closed http2 streams#23134
davedoesdev wants to merge 4 commits into
nodejs:masterfrom
davedoesdev:issue-23116-forget-closed-http2-streams

Conversation

davedoesdev commented Sep 27, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor

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 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 commented Sep 28, 2018

Copy link
Copy Markdown
Member

@nodejs/http2

jasnell commented Sep 28, 2018

Copy link
Copy Markdown
Member

Lgtm with green ci

addaleax commented Sep 28, 2018

Copy link
Copy Markdown
Member

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

addaleax added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Sep 28, 2018

davedoesdev commented Sep 28, 2018

Copy link
Copy Markdown
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 commented Sep 28, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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

mcollina approved these changes Sep 28, 2018
View reviewed changes

mcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM

jasnell commented Sep 28, 2018

Copy link
Copy Markdown
Member

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

addaleax commented Sep 28, 2018

Copy link
Copy Markdown
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 commented Sep 28, 2018

Copy link
Copy Markdown
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 commented Oct 2, 2018

Copy link
Copy Markdown
Contributor

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

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

davedoesdev commented Oct 2, 2018

Copy link
Copy Markdown
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 approved these changes Oct 2, 2018
View reviewed changes
Comment thread test/parallel/test-http2-forget-closed-streams.js Outdated

lpinca Oct 2, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

lpinca commented Oct 2, 2018 •
edited
Loading

Copy link
Copy Markdown
Member

Are the CI tests usually 100% pass?

No, there are some flaky tests.

joyeecheung mentioned this pull request Oct 2, 2018
Closed

danbev commented Oct 3, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

danbev commented Oct 3, 2018

Copy link
Copy Markdown
Contributor

Landed in e8121d5.

danbev closed this Oct 3, 2018
danbev pushed a commit that referenced this pull request Oct 3, 2018
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
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 mentioned this pull request Oct 7, 2018
Merged
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
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 mentioned this pull request Oct 20, 2018
Closed
2 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

mcollina mcollina approved these changes
jasnell jasnell approved these changes
lpinca lpinca approved these changes

Assignees

No one assigned

Labels

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

Back | FazBrowse Home | New Git URL