test: improve flaky http2-stream-destroy-event-order #31590
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This doesn't properly fix the issue, it only significantly reduces (removes?) the flakiness of the test. The setTimeout must not be needed here. Specifically if the settings frame on the client is 'delayed' and only comes after we have already called `req.close(2)` then the 'close' event will be emitted before the stream has got a chance to write the rst to the underlying socket (even though FlushRstStream and SendPendingData have been called) and since we are destroying a session here it will never be written resulting in a 'clean' stream close on the server side and therefore a timeout because there will be no error.
|
CI: https://ci.nodejs.org/job/node-test-pull-request/28807/ |
|
😆 got hit by 3 other flaky tests Edit: stress test looks ok but some of the machines failed to build, should I just restart? |
I'm not sure what the expected state of compiling |
addaleax
added a commit
to addaleax/node
that referenced
this pull request
Feb 1, 2020
Alternative to nodejs#31590. It appears that the issue here is that the test falsely assumed that closing the client (which also currently destroys the socket rather than gracefully shutting down the connection) would still leave enough time for the server side to receive the stream error. Address that by explicitly waiting for the server side to receive the stream error before closing the client and the connection with it. Refs: nodejs#31590 Refs: nodejs#20750
3 tasks
|
I think #31610 fully fixes this, can you take a look? If my understanding of the situation is correct, then yes, this PR improves the situation by giving the server one more event loop turn to receive the relevant stream data, but I think explicitly waiting for it to arrive is also fine and more reliable. |
Trott
pushed a commit
to Trott/io.js
that referenced
this pull request
Feb 6, 2020
Alternative to nodejs#31590. It appears that the issue here is that the test falsely assumed that closing the client (which also currently destroys the socket rather than gracefully shutting down the connection) would still leave enough time for the server side to receive the stream error. Address that by explicitly waiting for the server side to receive the stream error before closing the client and the connection with it. Refs: nodejs#31590 Refs: nodejs#20750 PR-URL: nodejs#31610 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Denys Otrishko <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Rich Trott <[email protected]>
codebytere
pushed a commit
that referenced
this pull request
Feb 17, 2020
Alternative to #31590. It appears that the issue here is that the test falsely assumed that closing the client (which also currently destroys the socket rather than gracefully shutting down the connection) would still leave enough time for the server side to receive the stream error. Address that by explicitly waiting for the server side to receive the stream error before closing the client and the connection with it. Refs: #31590 Refs: #20750 PR-URL: #31610 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Denys Otrishko <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Rich Trott <[email protected]>
codebytere
pushed a commit
that referenced
this pull request
Mar 15, 2020
Alternative to #31590. It appears that the issue here is that the test falsely assumed that closing the client (which also currently destroys the socket rather than gracefully shutting down the connection) would still leave enough time for the server side to receive the stream error. Address that by explicitly waiting for the server side to receive the stream error before closing the client and the connection with it. Refs: #31590 Refs: #20750 PR-URL: #31610 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Denys Otrishko <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Rich Trott <[email protected]>
codebytere
pushed a commit
that referenced
this pull request
Mar 17, 2020
Alternative to #31590. It appears that the issue here is that the test falsely assumed that closing the client (which also currently destroys the socket rather than gracefully shutting down the connection) would still leave enough time for the server side to receive the stream error. Address that by explicitly waiting for the server side to receive the stream error before closing the client and the connection with it. Refs: #31590 Refs: #20750 PR-URL: #31610 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Denys Otrishko <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Rich Trott <[email protected]>
codebytere
pushed a commit
that referenced
this pull request
Mar 30, 2020
Alternative to #31590. It appears that the issue here is that the test falsely assumed that closing the client (which also currently destroys the socket rather than gracefully shutting down the connection) would still leave enough time for the server side to receive the stream error. Address that by explicitly waiting for the server side to receive the stream error before closing the client and the connection with it. Refs: #31590 Refs: #20750 PR-URL: #31610 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Denys Otrishko <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Rich Trott <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This doesn't properly fix the issue, it only significantly reduces
(removes?) the flakiness of the test.
The setTimeout must not be needed here. Specifically if the settings
frame on the client is 'delayed' and only comes after we have already
called
req.close(2)then the 'close' event will be emitted before thestream has got a chance to write the rst to the underlying socket (even
though FlushRstStream and SendPendingData have been called) and since we
are destroying a session here it will never be written resulting in a
'clean' stream close on the server side and therefore a timeout because
there will be no error.
Looks like we are not correctly waiting for the rst frame to be sent and I assume the time it takes to read the settings frame delays the write just enough to not get written at all. I will try to investigate this further but at least this should improve our CI so that it won't fail with this test almost every time.
This patch reduces the flakiness from 1 failed in 10 runs to 0/2000 on my windows machine.
The debug native logs can be found:
http2-destroy-event-order-success.txt
http2-destroy-event-order-fail.txt.
/cc @nodejs/testing @jasnell @addaleax
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes