Skip to content

http: emit 'drain' after flushing coalesced chunks - #66782

Open
junyup0503 wants to merge 2 commits into
nodejs:mainfrom
junyup0503:http-drain-multibyte
Open

junyup0503 wants to merge 2 commits into
nodejs:mainfrom
junyup0503:http-drain-multibyte

Conversation

@junyup0503

Copy link
Copy Markdown

Since #64987, chunked writes made while the response is auto-corked are coalesced, and write() returns false based on the byte length of the buffered chunks. When uncork() flushes them, net.Socket counts string chunks in UTF-16 code units, so with multi-byte text the socket can stay below its own highWaterMark and never emits 'drain'. If the flushed writes complete asynchronously, the check at the end of uncork() also runs while bytes are still pending, so the response stalls with writableNeedDrain === true and writableLength === 0.

This re-checks the drain condition in the write callback of the flushed data, unless the message or its socket has been destroyed. It does not change the accounting itself (e.g. encoding the coalesced strings to Buffers), to avoid adding cost to the write path that #64987 optimized.

The new test uses a Duplex that counts strings like net.Socket and completes writes asynchronously, so it does not depend on OS socket buffer sizes. It hangs on main without the change and passes with it. It also checks that 'drain' is not emitted after the response is destroyed from a write callback. parallel/test-http*, parallel/test-stream*, parallel/test-net*, sequential/test-http* and make lint-js pass locally.

Fixes: #66781
Refs: #64987

I used an AI coding agent to help investigate and write this change. I reproduced the issue and verified the fix and the tests myself.

Since chunked writes are coalesced while the message is corked,
write() decides backpressure from the byte length of the buffered
chunks and sets kNeedDrain. When uncork() flushes them, net.Socket
accounts string chunks in UTF-16 code units, so with multi-byte text it
can stay below its own highWaterMark. It then never needs to drain and
never emits 'drain'. If the flushed writes complete asynchronously, the
check at the end of uncork() also runs while bytes are still pending,
so the response stalls with writableNeedDrain === true and
writableLength === 0.

Re-check the drain condition once the flushed data has been written,
unless the message or its socket has been destroyed in the meantime.

Fixes: nodejs#66781
Refs: nodejs#64987
Assisted-by: AI coding agent
Signed-off-by: junyup0503 <[email protected]>
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/http
  • @nodejs/net

@nodejs-github-bot nodejs-github-bot added http Issues and PRs related to the http subsystem. needs-ci PRs that need a full CI run. labels Oct 16, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Welcome to Node.js, and thank you for your first contribution!

Before review, please take a moment to read:

Please make sure every commit is signed off. For a first pull request, GitHub Actions require collaborator approval and Jenkins CI must be started by a collaborator or triager, so an initial wait is normal.

@par333k

par333k commented Oct 16, 2026 •

Copy link
Copy Markdown
const currentSocket = this[kSocket]

The socket reference is captured when uncork() runs, so it is null if the message had no socket yet and the write was buffered in outputData. Re-reading this[kSocket] in the callback checks the socket the data was actually flushed to. In practice _flush() usually clears kNeedDrain before this callback runs, so this is a defensive guard rather than a fix for an observed crash.

Comment thread lib/internal/http/outgoing.js Outdated
Comment on lines +382 to +384
// Re-check once the flushed data has been written.
if (!err && this.writableNeedDrain && !socket.destroyed &&
this.writableLength === 0) {

@par333k par333k Oct 16, 2026 •

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggested change
// Re-check once the flushed data has been written.
if (!err && this.writableNeedDrain && !socket.destroyed &&
this.writableLength === 0) {
const currentSocket = this[kSocket];
if (!err && this.writableNeedDrain && currentSocket &&
!currentSocket.destroyed && this.writableLength === 0) {

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.

+1, I think without this there is likely a reachable crash here, good catch @par333k

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Thanks for the suggestion! @par333k
Applied in 96a0377. The regression test and five related HTTP tests all pass.

@codecov

codecov Bot commented Oct 26, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.67%. Comparing base (1c90519) to head (96a0377).
⚠️ Report is 223 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #66782      +/-   ##
==========================================
+ Coverage   90.49%   90.67%   +0.17%     
==========================================
  Files         797      797              
  Lines      277598   278783    +1185     
  Branches    53336    53611     +275     
==========================================
+ Hits       251220   252779    +1559     
+ Misses      16790    16359     -431     
- Partials     9588     9645      +57     
Files with missing lines Coverage Δ
lib/internal/http/outgoing.js 97.99% <100.00%> (+0.01%) ⬆️

... and 121 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

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

The comment here needs resolving, but otherwise I think this is good to go and fixes a real regression. Thanks for the fix @junyup0503!

Comment thread lib/internal/http/outgoing.js Outdated
Comment on lines +382 to +384
// Re-check once the flushed data has been written.
if (!err && this.writableNeedDrain && !socket.destroyed &&
this.writableLength === 0) {

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.

+1, I think without this there is likely a reachable crash here, good catch @par333k

Read the current socket after the coalesced write callbacks run, since
uncork() may initially buffer data before a socket is assigned. Check
that the current socket exists and is not destroyed before emitting
drain.

Refs: nodejs#66782 (comment)
Assisted-by: AI coding agent
Signed-off-by: junyup0503 <[email protected]>
@pimterry pimterry added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Oct 30, 2026
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Oct 30, 2026
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@junyup0503

Copy link
Copy Markdown
Author

I applied @par333k’s suggestion locally but missed the co-author attribution. Could we include them as a co-author in the final commit when landing this PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

http Issues and 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.

http: 'drain' is never emitted for chunked responses with multi-byte strings (v24.22.0 regression)

5 participants