Skip to content

http: fix drain event with cork/uncork#64038

Open
davidje13 wants to merge 2 commits into
nodejs:mainfrom
davidje13:cork-drain
Open

http: fix drain event with cork/uncork#64038
davidje13 wants to merge 2 commits into
nodejs:mainfrom
davidje13:cork-drain

Conversation

@davidje13

Copy link
Copy Markdown

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() 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.

The Problem:

  • When corked, write() buffers data in kChunkedBuffer instead of sending immediately
  • If write() returns false, kNeedDrain is set to true
  • When uncork() is called, it flushes the buffer by calling _send()
  • After flushing, uncork() never checked if drain was needed or emitted the event
  • This left the response waiting for a drain event that would never fire

The Fix:
This commit ensures that when uncork() successfully flushes buffered data and a drain was needed, the drain event is emitted immediately.

Changes:

  • Modified OutgoingMessage.prototype.uncork() in lib/_http_outgoing.js
  • Capture return value from final _send() call
  • Check if drain was pending after successful flush
  • Emit drain event when appropriate

Testing:
The issue could be reproduced by using cork() with multiple large writes and checking for drain events. With this fix, the drain event now fires correctly after uncork().

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/http
  • @nodejs/net

@nodejs-github-bot nodejs-github-bot added http Issues or PRs related to the http subsystem. needs-ci PRs that need a full CI run. labels Jun 21, 2026
@ronag ronag requested a review from Copilot June 21, 2026 11:30

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.

Comment thread test/parallel/test-http-response-drain-cork.js Outdated
Comment thread test/parallel/test-http-response-drain-cork.js Outdated
@ronag

ronag commented Jun 21, 2026

Copy link
Copy Markdown
Member

Please fix copilot feedback.

Comment thread lib/_http_outgoing.js Outdated
Comment thread lib/_http_outgoing.js Outdated
@davidje13

Copy link
Copy Markdown
Author

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.

@ronag ronag added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 21, 2026
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 21, 2026
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@Renegade334 Renegade334 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jun 21, 2026

@mcollina mcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm

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.
@davidje13

Copy link
Copy Markdown
Author

Amended first commit message to fix commit lint error (also rebased on latest main)

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. http Issues or PRs related to the http subsystem. needs-ci PRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

drain event is unreliable when using cork/uncork with ServerResponse

6 participants