-
-
Notifications
You must be signed in to change notification settings - Fork 35k
test: fix flaky test-http2-close-while-writing#61804
test: fix flaky test-http2-close-while-writing#61804Han5991 wants to merge 1 commit intonodejs:mainfrom
Conversation
The test was flaky due to a race condition where client.close() would
wait for a graceful shutdown while the server had already destroyed the
session/stream, leading to a timeout.
Changes:
- Replace
client.close()withclient.destroy()in thecloseevent
handler of the client stream. - Add
common.mustNotCall()error handlers to the client, client stream,
and server session to catch unexpected errors. - Add check
if (!client_stream.destroyed)before destroying client
stream in the server's data handler.
Refs: #33156
| client_stream.on('error', common.mustNotCall()); | ||
| client_stream.on('close', common.mustCall(() => { | ||
| client.close(); | ||
| client.destroy(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is still not clear to me why this callback
node/lib/internal/http2/core.js
Lines 2192 to 2196 in 6710c00
| const writeCallback = (err) => { | |
| waitingForWriteCallback = false; | |
| writeCallbackErr = err; | |
| done(); | |
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with your suggestion. Using destroy() feels more natural here given the test scenario involves an abrupt disconnection while writing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This hides the bug, the callback should be called.
Codecov Report All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #61804 +/- ## ========================================== - Coverage 89.75% 89.75% -0.01% ========================================== Files 675 675 Lines 204676 204735 +59 Branches 39329 39341 +12 ========================================== + Hits 183705 183752 +47 + Misses 13272 13269 -3 - Partials 7699 7714 +15 New features to boost your workflow:
|
df7750e to
bcab32d
Compare
wait for a graceful shutdown while the server had already destroyed the
session/stream, leading to a timeout.
Changes:
- Replace `client.close()` with `client.destroy()` in the `close`
event handler of the client stream.
Refs: nodejs#33156
bcab32d to
4bc4b5d
Compare