Skip to content

http: drain server request before destroying - #65674

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

http: drain server request before destroying#65674
dayun6530 wants to merge 1 commit into
nodejs:mainfrom
dayun6530:fix/http-stream-early-termination

Conversation

@dayun6530

Copy link
Copy Markdown
Contributor

Refs: #49429

When a server IncomingMessage is consumed using an async iterator and
iteration terminates before the request body is fully consumed, the
IncomingMessage is destroyed while the underlying keep-alive socket
remains open.

Pending request body data can then be pushed to the destroyed stream.
push() returns false, causing the HTTP parser to pause the socket.
A subsequent request that reuses the same keep-alive socket can then stall.

This change dumps the remaining server request body before detaching and
destroying the IncomingMessage.

A regression test verifies that a subsequent request can reuse the same
keep-alive socket without timing out.

Tests:

  • python3 tools/test.py --mode=release parallel/test-http-server-for-await-keepalive
  • python3 tools/test.py --mode=release parallel/test-http-client-abort-keep-alive-destroy-res parallel/test-http-agent-keepalive
  • python3 tools/test.py --mode=release parallel/test-stream-readable-async-iterators parallel/test-stream-pipeline-async-iterator
  • make lint-js

Signed-off-by: Dayun <dlekdbs6530@gmail.com>
@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.05%. Comparing base (bb5cffc) to head (6d8cf43).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #65674      +/-   ##
==========================================
- Coverage   90.06%   90.05%   -0.01%     
==========================================
  Files         754      754              
  Lines      255747   255748       +1     
  Branches    48318    48327       +9     
==========================================
- Hits       230327   230309      -18     
+ Misses      16550    16546       -4     
- Partials     8870     8893      +23     
Files with missing lines Coverage Δ
lib/internal/streams/destroy.js 93.64% <100.00%> (+0.01%) ⬆️

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

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