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

test: fix flaky parallel/test-http2-client-upload #17361

Closed
wants to merge 1 commit into from
Closed

test: fix flaky parallel/test-http2-client-upload #17361

wants to merge 1 commit into from

Conversation

addaleax
Copy link
Member

In parallel/test-http2-client-upload, the client.destroy()
call could terminate the connection before all data was sent
over the wire successfully.

Using client.shutdown() removes the flakiness.

Also, listen on req.on('finish') rather than the file stream’s
end event, since we’re not interested in when the source stream
finishes, but rather when the HTTP/2 stream finishes.

Refs: #17356

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

test/http2

@addaleax
test: fix flaky parallel/test-http2-client-upload
31861a7 
In parallel/test-http2-client-upload, the `client.destroy()`
call could terminate the connection before all data was sent
over the wire successfully.

Using `client.shutdown()` removes the flakiness.

Also, listen on `req.on('finish')` rather than the file stream’s
`end` event, since we’re not interested in when the source stream
finishes, but rather when the HTTP/2 stream finishes.

Refs: nodejs#17356
@addaleax addaleax added 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. labels Nov 27, 2017
@addaleax addaleax requested a review from jasnell November 27, 2017 23:20
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Nov 27, 2017
@addaleax addaleax mentioned this pull request Nov 27, 2017
Closed
3 tasks
@addaleax
Copy link
Member Author

addaleax commented Nov 27, 2017 •
edited
Loading

Stress test: https://ci.nodejs.org/job/node-stress-single-test/1555/nodes=win2008r2-mp-vs2017/console (☑️)
CI: https://ci.nodejs.org/job/node-test-commit/14410/

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 27, 2017
@addaleax
Copy link
Member Author

I’d like to fast-track this, whether #17356 lands or not.

@addaleax
Copy link
Member Author

Landed in 6fc8765

@addaleax addaleax closed this Nov 28, 2017
@addaleax addaleax deleted the fix-test-http2-client-upload branch November 28, 2017 01:37
addaleax added a commit that referenced this pull request Nov 28, 2017
@addaleax
test: fix flaky parallel/test-http2-client-upload
6fc8765 
In parallel/test-http2-client-upload, the `client.destroy()`
call could terminate the connection before all data was sent
over the wire successfully.

Using `client.shutdown()` removes the flakiness.

Also, listen on `req.on('finish')` rather than the file stream’s
`end` event, since we’re not interested in when the source stream
finishes, but rather when the HTTP/2 stream finishes.

PR-URL: #17361
Refs: #17356
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Kyle Farnung <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
@addaleax addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 28, 2017
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
@addaleax @MylesBorins
test: fix flaky parallel/test-http2-client-upload
17f39b2 
In parallel/test-http2-client-upload, the `client.destroy()`
call could terminate the connection before all data was sent
over the wire successfully.

Using `client.shutdown()` removes the flakiness.

Also, listen on `req.on('finish')` rather than the file stream’s
`end` event, since we’re not interested in when the source stream
finishes, but rather when the HTTP/2 stream finishes.

PR-URL: #17361
Refs: #17356
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Kyle Farnung <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
@addaleax @MylesBorins
test: fix flaky parallel/test-http2-client-upload
ee25bc1 
In parallel/test-http2-client-upload, the `client.destroy()`
call could terminate the connection before all data was sent
over the wire successfully.

Using `client.shutdown()` removes the flakiness.

Also, listen on `req.on('finish')` rather than the file stream’s
`end` event, since we’re not interested in when the source stream
finishes, but rather when the HTTP/2 stream finishes.

PR-URL: #17361
Refs: #17356
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Kyle Farnung <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
@addaleax @MylesBorins
test: fix flaky parallel/test-http2-client-upload
1e3aed0 
In parallel/test-http2-client-upload, the `client.destroy()`
call could terminate the connection before all data was sent
over the wire successfully.

Using `client.shutdown()` removes the flakiness.

Also, listen on `req.on('finish')` rather than the file stream’s
`end` event, since we’re not interested in when the source stream
finishes, but rather when the HTTP/2 stream finishes.

PR-URL: #17361
Refs: #17356
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Kyle Farnung <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Dec 12, 2017
Merged
gibfahn pushed a commit that referenced this pull request Dec 19, 2017
@addaleax @gibfahn
test: fix flaky parallel/test-http2-client-upload
0b45b42 
In parallel/test-http2-client-upload, the `client.destroy()`
call could terminate the connection before all data was sent
over the wire successfully.

Using `client.shutdown()` removes the flakiness.

Also, listen on `req.on('finish')` rather than the file stream’s
`end` event, since we’re not interested in when the source stream
finishes, but rather when the HTTP/2 stream finishes.

PR-URL: #17361
Refs: #17356
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Kyle Farnung <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
@gibfahn gibfahn mentioned this pull request Dec 20, 2017
Closed
gibfahn pushed a commit that referenced this pull request Dec 20, 2017
@addaleax @gibfahn
test: fix flaky parallel/test-http2-client-upload
341f0c5 
In parallel/test-http2-client-upload, the `client.destroy()`
call could terminate the connection before all data was sent
over the wire successfully.

Using `client.shutdown()` removes the flakiness.

Also, listen on `req.on('finish')` rather than the file stream’s
`end` event, since we’re not interested in when the source stream
finishes, but rather when the HTTP/2 stream finishes.

PR-URL: #17361
Refs: #17356
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Kyle Farnung <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
@gibfahn gibfahn mentioned this pull request Dec 20, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maclover7 maclover7 maclover7 approved these changes

@kfarnung kfarnung kfarnung approved these changes

@jasnell jasnell jasnell approved these changes

@refack refack refack approved these changes

@apapirovski apapirovski apapirovski approved these changes

Assignees
No one assigned
Labels
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.
8 participants
@addaleax @apapirovski @refack @jasnell @kfarnung @maclover7 @gibfahn @nodejs-github-bot