Skip to content

test: improve flaky http2-stream-destroy-event-order #31590

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

Closed
lundibundi wants to merge 1 commit into from
+10 −1
Closed

test: improve flaky http2-stream-destroy-event-order #31590

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
test: improve flaky http2-stream-destroy-event-order 
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.
  • Loading branch information
@lundibundi
lundibundi committed Jan 31, 2020
commit 5c0eab9496a25494da203a0beb3c0ec29589a04a
11 changes: 10 additions & 1 deletion test/parallel/test-http2-stream-destroy-event-order.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,16 @@ server.listen(0, common.mustCall(() => {
req.resume();
req.on('error', common.mustCall(() => {
req.on('close', common.mustCall(() => {
client.close();
// TODO(lundibundi): fix flakiness of this use case, 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.
setTimeout(() => client.close(), 0);
}));
}));
}));