Skip to content

http2: emit timeout on compat request and response#22252

Closed
jasnell wants to merge 2 commits intonodejs:masterfrom
jasnell:http2-compat-emit-timeout
Closed

http2: emit timeout on compat request and response#22252
jasnell wants to merge 2 commits intonodejs:masterfrom
jasnell:http2-compat-emit-timeout

Conversation

@jasnell
Copy link
Member

@jasnell jasnell commented Aug 10, 2018

Alternative for: #20918

Fixes: #20079

ping @mcollina @apapirovski

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

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Aug 10, 2018

@jasnell build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/509/pipeline

@nodejs-github-bot nodejs-github-bot added dont-land-on-v6.x http2 Issues or PRs related to the http2 subsystem. labels Aug 10, 2018
@jasnell jasnell force-pushed the http2-compat-emit-timeout branch from 2b4cdd8 to e8f319f Compare August 10, 2018 20:37
@jasnell
Copy link
Member Author

jasnell commented Aug 10, 2018

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

@jasnell
Copy link
Member Author

jasnell commented Aug 11, 2018

Rerun linter: https://ci.nodejs.org/job/node-test-linter/21196/

@jasnell jasnell added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 11, 2018
@Trott Trott removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 11, 2018
@Trott
Copy link
Member

Trott commented Aug 11, 2018

Please don't add author ready label until there is at least one Collaborator approval

mcollina
mcollina approved these changes Aug 11, 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

@mcollina mcollina added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 11, 2018
@Trott Trott mentioned this pull request Aug 12, 2018
Closed
jasnell added a commit that referenced this pull request Aug 15, 2018
@jasnell
http2: emit timeout on compat request and response
32902d0 
Fixes: #20079

PR-URL: #22252
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@jasnell
Copy link
Member Author

jasnell commented Aug 15, 2018

Landed in 32902d0 ... successful ci runs: #22252 (comment) + https://ci.nodejs.org/job/node-test-linter/21196/

@jasnell jasnell closed this Aug 15, 2018
targos pushed a commit that referenced this pull request Aug 19, 2018
@jasnell @targos
http2: emit timeout on compat request and response
384e881 
Fixes: #20079

PR-URL: #22252
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
targos pushed a commit that referenced this pull request Sep 3, 2018
@jasnell @targos
http2: emit timeout on compat request and response
d3ceaa1 
Fixes: #20079

PR-URL: #22252
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@targos targos mentioned this pull request Sep 5, 2018
Merged
This was referenced Sep 6, 2018
Closed
Closed
@jasnell jasnell mentioned this pull request Sep 10, 2018
Closed
4 tasks
@kjin
Copy link
Contributor

kjin commented Oct 3, 2018

Hi @jasnell -- working on backporting this fix. It seems like in Node 8, the 'timeout' event listeners added to the tests in this PR are often called more than once, I believe because res.end() resets the timeout. I don't see anything in Node 10 that safeguards against this in particular; given this, I was wondering if it is acceptable to allow 'timeout' to be called at least once instead of exactly once.

kjin pushed a commit to kjin/node that referenced this pull request Oct 3, 2018
@jasnell @kjin
http2: emit timeout on compat request and response
c4e0a27 
Fixes: nodejs#20079

PR-URL: nodejs#22252
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@kjin kjin mentioned this pull request Oct 3, 2018
Closed
4 tasks
@mcollina
Copy link
Member

mcollina commented Oct 4, 2018

@kjin I think this shows some underlining fragility of those tests. I would prefer to increase that timeout to 10. Those comment should be added to master as well.

@kjin
Copy link
Contributor

kjin commented Oct 4, 2018

@mcollina Thanks. I'll open a PR to add that to master, and annotate the backported version of this commit in the 8.x backport to increase the timeout as well.

kjin pushed a commit to kjin/node that referenced this pull request Oct 16, 2018
@jasnell @kjin
http2: emit timeout on compat request and response
3dc84ca 
v8.x Backport Note: The timeout has been increased to 10ms.

Fixes: nodejs#20079

PR-URL: nodejs#22252
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
BethGriggs pushed a commit that referenced this pull request Oct 17, 2018
@jasnell @BethGriggs
http2: emit timeout on compat request and response
348cde0 
v8.x Backport Note: The timeout has been increased to 10ms.

Fixes: #20079

Backport-PR-URL: #22850
PR-URL: #22252
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@BethGriggs BethGriggs mentioned this pull request Oct 30, 2018
Merged
@beevelop beevelop mentioned this pull request Nov 20, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mcollina mcollina mcollina approved these changes

@trivikr trivikr trivikr 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. 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.

6 participants

@jasnell @nodejs-github-bot @Trott @kjin @mcollina @trivikr