Skip to content
New issue

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

#

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

Already on GitHub? # to your account

Jump to bottom

test: fix race in test-http2-origin #28903

Closed
wants to merge 1 commit into from
Closed

test: fix race in test-http2-origin #28903

wants to merge 1 commit into from

Conversation

mildsunrise
Copy link
Member

#27861 has uncovered another race in test-http2-origin. If origin fires too soon, the session will be closed while the request is in progress:

Error [ERR_HTTP2_STREAM_ERROR]: Stream closed with error code NGHTTP2_REFUSED_STREAM
    at ClientHttp2Stream._destroy (internal/http2/core.js:1974:13)
    at ClientHttp2Stream.destroy (internal/streams/destroy.js:37:8)
    at ClientHttp2Stream.[kMaybeDestroy] (internal/http2/core.js:1990:12)
    at Http2Stream.onStreamClose (internal/http2/core.js:391:26)
Emitted 'error' event at:
    at emitErrorNT (internal/streams/destroy.js:91:8)
    at emitErrorAndCloseNT (internal/streams/destroy.js:59:3)
    at processTicksAndRejections (internal/process/task_queues.js:77:11)
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Jul 30, 2019
@mildsunrise
Copy link
Member Author

mildsunrise commented Jul 30, 2019 •
edited
Loading

@addaleax This is blocking the backport for #27861, should I just include these changes in the backport and submit it now? Or should I wait until this lands and cherry-pick it in the backport?

@addaleax addaleax added the http2 Issues or PRs related to the http2 subsystem. label Jul 30, 2019
addaleax
addaleax approved these changes Jul 30, 2019
View reviewed changes
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

@jmendeth Thank you for taking this on! Since this is a test change, I think what I would suggest would be to land this as soon as possible, and then afterwards you can include it as a cherry-pick in the backport PR – that works, right?

@nodejs-github-bot
Copy link
Collaborator

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

@addaleax addaleax added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. fast-track PRs that do not need to wait for 48 hours to land. labels Jul 30, 2019
@addaleax
Copy link
Member

Collaborators, feel free to 👍 this comment to approve fast-tracking.

@mildsunrise
Copy link
Member Author

Great, thanks!!

@Trott
Copy link
Member

Trott commented Jul 30, 2019

Landed in 985c5f5

@Trott Trott closed this Jul 30, 2019
Trott pushed a commit to Trott/io.js that referenced this pull request Jul 30, 2019
@mildsunrise @Trott
test: fix race in test-http2-origin
985c5f5 
PR-URL: nodejs#28903
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
mildsunrise added a commit to mildsunrise/node that referenced this pull request Jul 30, 2019
@mildsunrise
test: fix race in test-http2-origin
a7b4a6f 
PR-URL: nodejs#28903
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@mildsunrise mildsunrise deleted the patch-1 branch July 30, 2019 21:33
@mildsunrise mildsunrise mentioned this pull request Jul 30, 2019
Closed
targos pushed a commit that referenced this pull request Aug 2, 2019
@mildsunrise @targos
test: fix race in test-http2-origin
31aa33b 
PR-URL: #28903
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@BridgeAR BridgeAR mentioned this pull request Aug 6, 2019
Merged
BethGriggs pushed a commit that referenced this pull request Oct 7, 2019
@mildsunrise @BethGriggs
test: fix race in test-http2-origin
f78ecc3 
Backport-PR-URL: #28904
PR-URL: #28903
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@BethGriggs BethGriggs mentioned this pull request Oct 7, 2019
Merged
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@Trott Trott Trott approved these changes

@addaleax addaleax addaleax 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. fast-track PRs that do not need to wait for 48 hours to land. http2 Issues or PRs related to the http2 subsystem. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mildsunrise @nodejs-github-bot @addaleax @Trott