Skip to content

http: prevent reuse after incomplete request destruction - #65674

Open
dayun6530 wants to merge 1 commit into
nodejs:mainfrom
dayun6530:fix/http-stream-early-termination
Open

http: prevent reuse after incomplete request destruction#65674
dayun6530 wants to merge 1 commit into
nodejs:mainfrom
dayun6530:fix/http-stream-early-termination

Conversation

@dayun6530

@dayun6530 dayun6530 commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Refs: #49429

When a server IncomingMessage is consumed with an async iterator and iteration terminates before the request body is fully consumed, the stream destroy path detaches the request from its socket before destroying it. This leaves the underlying keep-alive connection open even though the request was not fully consumed.

Unread request body data can then reach the destroyed stream, pause the socket, and make a subsequent request that reuses the same connection stall.

This change prevents that connection from being reused:

  • If the current response headers have not been sent yet, the response is marked non-keep-alive and as the last response on the connection. The current response can still complete, but the client receives Connection: close and the socket is not reused.
  • If the response headers have already been sent, the response is destroyed. At that point the keep-alive decision has already been committed, so destroying the response/socket avoids reusing a connection whose request was not fully consumed.

The request stream is then detached and destroyed as before.

Regression tests cover both cases:

  • headers not sent: the first response completes with Connection: close, and the next request uses a new socket;
  • headers already sent: the first response is aborted with ECONNRESET, and the next request uses a new socket.

Tests:

  • make lint-js
  • python3 tools/test.py --mode=release test/parallel/test-http-server-for-await-keepalive.js test/parallel/test-http-server-for-await-keepalive-headers-sent.js
  • python3 tools/test.py --mode=release test/parallel/test-stream-destroy.js test/parallel/test-http-server-incomingmessage-destroy.js test/parallel/test-http-incoming-pipelined-socket-destroy.js

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/streams

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. stream Issues and PRs related to Node.js streams. labels Aug 31, 2026
@dayun6530
dayun6530 marked this pull request as ready for review August 31, 2026 01:04
@codecov

codecov Bot commented Aug 31, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.19%. Comparing base (bb5cffc) to head (9573192).
⚠️ Report is 171 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #65674      +/-   ##
==========================================
+ Coverage   90.06%   90.19%   +0.13%     
==========================================
  Files         754      771      +17     
  Lines      255747   264631    +8884     
  Branches    48318    50233    +1915     
==========================================
+ Hits       230327   238689    +8362     
- Misses      16550    16945     +395     
- Partials     8870     8997     +127     
Files with missing lines Coverage Δ
lib/internal/streams/destroy.js 93.83% <100.00%> (+0.20%) ⬆️

... and 166 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.

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

Something more fundamental is wrong here. The socket should not be re-used if destroyed without being fully consumed and anything else in the pipeline queue should also be cancelled.

@ronag

ronag commented Aug 31, 2026

Copy link
Copy Markdown
Member

@mcollina

Signed-off-by: Dayun <dlekdbs6530@gmail.com>
@dayun6530
dayun6530 force-pushed the fix/http-stream-early-termination branch from 6d8cf43 to 9573192 Compare September 6, 2026 22:57
@dayun6530 dayun6530 changed the title http: drain server request before destroying http: prevent reuse after incomplete request destruction Sep 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-ci PRs that need a full CI run. stream Issues and PRs related to Node.js streams.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants