http: fix drain event with cork/uncork#64038
Conversation
|
Review requested:
|
There was a problem hiding this comment.
Pull request overview
This pull request fixes a reliability issue where ServerResponse could get stuck with writableNeedDrain === true when using cork()/uncork(), because uncork() flushed the internal chunked buffer without emitting 'drain' when no socket-level backpressure occurred.
Changes:
- Update
OutgoingMessage.prototype.uncork()to emit'drain'after flushing the chunked buffer when a drain was pending. - Add a regression test covering
ServerResponse'drain'behavior when corked writes hit backpressure.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
lib/_http_outgoing.js |
Emits 'drain' from uncork() when the buffered chunked payload is flushed successfully and kNeedDrain was set. |
test/parallel/test-http-response-drain-cork.js |
Adds a regression test for the cork/uncork drain behavior on ServerResponse. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Please fix copilot feedback. |
|
Applied the requested changes, and simplified the test a little since setting an explicit high water mark means we don't need to be so extreme with the volume of data. I also removed the '3' part of the test, which I don't think was testing anything useful here, and is likely just a remnant of how I originally diagnosed the issue. |
When using cork() and uncork() with ServerResponse, the drain event was not reliably emitted after uncorking. This occurred because the uncork() method did not check if a drain was pending (kNeedDrain flag) after flushing the chunked buffer. This fix ensures that when uncork() successfully flushes buffered data and a drain was needed, the drain event is emitted immediately. This commit is a copy of PR nodejs#60437 (abandoned) with minor linting fixes. Fixes: nodejs#60432 Signed-off-by: David Evans <davidje13@users.noreply.github.com>
Check internal state when deciding whether to send a drain event on uncork() - instead of relying on the return value of _send - as suggested in the PR comments. The test is updated to set an explicit high water mark for better reliability, and since this means we can use a much lower value, the size of data in the test has been reduced significantly.
|
Amended first commit message to fix commit lint error (also rebased on latest main) |
Fixes #60432
This is a copy of PR #60437, so credit for the fix goes to @ThierryMT. Sadly that PR was abandoned and I wasn't able to get a response from the author. I've just applied their changes to the latest main, patched up the lint errors, and applied the suggested change to the tests. I've signed the certificate of origin on the basis that the test is based on the reproduction I provided in the original bug report #60432 (hence, created in part by me), but let me know if this is too tenuous and I can write a new test from scratch instead. The fix itself is so trivial that I'm not sure there's a way to write it which isn't effectively the same code.
Original PR description:
When using
cork()anduncork()withServerResponse, thedrainevent was not reliably emitted after uncorking. This occurred because theuncork()method did not check if a drain was pending (kNeedDrainflag) after flushing the chunked buffer.The Problem:
write()buffers data inkChunkedBufferinstead of sending immediatelywrite()returnsfalse,kNeedDrainis set totrueuncork()is called, it flushes the buffer by calling_send()uncork()never checked if drain was needed or emitted the eventdrainevent that would never fireThe Fix:
This commit ensures that when
uncork()successfully flushes buffered data and a drain was needed, thedrainevent is emitted immediately.Changes:
OutgoingMessage.prototype.uncork()inlib/_http_outgoing.js_send()calldrainevent when appropriateTesting:
The issue could be reproduced by using
cork()with multiple large writes and checking fordrainevents. With this fix, thedrainevent now fires correctly afteruncork().