Skip to content

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

Open
junyup0503 wants to merge 1 commit into
nodejs:mainfrom
junyup0503:http-drain-multibyte
Open

junyup0503 wants to merge 1 commit 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 <97156898+junyup0503@users.noreply.github.com>
@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 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) {

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)

3 participants