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.

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

test: fix flaky http-client-timeout-agent #19856

Conversation

santigimeno
Copy link
Member

Close the connection immediately after receiving the end event to
avoid that the timeout event is fired before the connection closes
gracefully.

Fixes: #19804

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

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Apr 6, 2018
Copy link
Member

@lpinca lpinca left a comment

Choose a reason for hiding this comment

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

SGTM

@santigimeno
Copy link
Member Author

@lpinca
Copy link
Member

lpinca commented Apr 6, 2018

If 'timeout' can be emitted after 'end' isn't this a bug?

@santigimeno
Copy link
Member Author

I guess it may make sense not to clear the timeout until the connection is fully closed and not only when the other end closes its side.

Close the connection immediately after receiving the `end` event to
avoid that the `timeout` event is fired before the connection closes
gracefully.
Move the test back to parallel.

Fixes: nodejs#19804
@santigimeno santigimeno force-pushed the fix_test-http-client-timeout-agent branch from dd7b621 to 3f2078f Compare April 7, 2018 11:23
@santigimeno
Copy link
Member Author

Updated so the test is moved back to parallel as 95dc590 moved it to sequential.

@BridgeAR
Copy link
Member

BridgeAR commented Apr 8, 2018

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 8, 2018
@BridgeAR
Copy link
Member

BridgeAR commented Apr 9, 2018

Landed in 5b8c62c 🎉

@BridgeAR BridgeAR closed this Apr 9, 2018
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Apr 9, 2018
Close the connection immediately after receiving the `end` event to
avoid that the `timeout` event is fired before the connection closes
gracefully.
Move the test back to parallel.

PR-URL: nodejs#19856
Fixes: nodejs#19804
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
targos pushed a commit that referenced this pull request Apr 12, 2018
Close the connection immediately after receiving the `end` event to
avoid that the `timeout` event is fired before the connection closes
gracefully.
Move the test back to parallel.

PR-URL: #19856
Fixes: #19804
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request May 1, 2018
Close the connection immediately after receiving the `end` event to
avoid that the `timeout` event is fired before the connection closes
gracefully.
Move the test back to parallel.

PR-URL: nodejs#19856
Fixes: nodejs#19804
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
MylesBorins pushed a commit that referenced this pull request Aug 1, 2018
Close the connection immediately after receiving the `end` event to
avoid that the `timeout` event is fired before the connection closes
gracefully.
Move the test back to parallel.

PR-URL: #19856
Fixes: #19804
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
rvagg pushed a commit that referenced this pull request Aug 16, 2018
Close the connection immediately after receiving the `end` event to
avoid that the `timeout` event is fired before the connection closes
gracefully.
Move the test back to parallel.

PR-URL: #19856
Fixes: #19804
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Aug 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

investigate flaky parallel/test-http-client-timeout-agent
6 participants