Skip to content

Conversation

@jasnell
Copy link
Member

Test recently started failing only in CI, unable to recreate locally on multiple test machines. Need to investigate further.

/cc @addaleax

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)

http2, test

Test recently started failing only in CI, unable to recreate locally on multiple test machines. Need to investigate further.
@nodejs-github-botnodejs-github-bot added the test Issues and PRs related to the tests. label Nov 27, 2017
@jasnell
Copy link
MemberAuthor

@addaleaxaddaleax added fast-track PRs that do not need to wait for 48 hours to land. http2 Issues or PRs related to the http2 subsystem. labels Nov 27, 2017
@jasnell
Copy link
MemberAuthor

Assuming CI comes back good, I'd like to fast track this to get CI green again.

Copy link
Contributor

@refackrefack left a comment

Choose a reason for hiding this comment

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

Just about to suggest exactly this

@refackrefack added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 27, 2017
@addaleaxaddaleax removed fast-track PRs that do not need to wait for 48 hours to land. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Nov 27, 2017
addaleax added a commit to addaleax/node that referenced this pull request Nov 27, 2017
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
Copy link
Member

Alternatively, here’s a fix: #17361

addaleax added a commit that referenced this pull request Nov 28, 2017
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
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
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
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 pushed a commit that referenced this pull request Dec 19, 2017
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 pushed a commit that referenced this pull request Dec 20, 2017
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]>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

http2Issues or PRs related to the http2 subsystem.testIssues and PRs related to the tests.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@jasnell@addaleax@refack@nodejs-github-bot