Skip to content

http: avoid aborting IncomingMessage signal on normal close - #64392

Open
Archkon wants to merge 1 commit into
nodejs:mainfrom
Archkon:httpfix
Open

http: avoid aborting IncomingMessage signal on normal close#64392
Archkon wants to merge 1 commit into
nodejs:mainfrom
Archkon:httpfix

Conversation

@Archkon

@Archkon Archkon commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Fixes: #64390

@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 Jul 9, 2026
@codecov

codecov Bot commented Jul 9, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 87.09677% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.13%. Comparing base (58795e9) to head (2ced6a6).
⚠️ Report is 12 commits behind head on main.

Files with missing lines Patch % Lines
lib/_http_incoming.js 85.18% 7 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #64392      +/-   ##
==========================================
- Coverage   90.16%   90.13%   -0.03%     
==========================================
  Files         744      744              
  Lines      242518   242575      +57     
  Branches    45692    45702      +10     
==========================================
- Hits       218663   218642      -21     
- Misses      15349    15445      +96     
+ Partials     8506     8488      -18     
Files with missing lines Coverage Δ
lib/_http_client.js 97.63% <100.00%> (+<0.01%) ⬆️
lib/_http_server.js 96.51% <100.00%> (-0.13%) ⬇️
lib/_http_incoming.js 97.91% <85.18%> (-1.46%) ⬇️

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

@Ethan-Arrowood Ethan-Arrowood left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

http.md:3052 says the signal "is aborted when the underlying socket closes or the request is destroyed." After this PR, neither is unconditionally true: normal completion detaches the socket listener, and destroy-after-completion no longer aborts. The PR touches no docs. It needs the prose rewritten (something like "aborted when the client disconnects before the message completes") plus a changes:/REPLACEME YAML entry.

Comment thread lib/_http_incoming.js Outdated
@Archkon

Archkon commented Jul 14, 2026

Copy link
Copy Markdown
Contributor Author

@Ethan-Arrowood Could you review the code again when available? Thanks !

@Archkon

Archkon commented Jul 18, 2026

Copy link
Copy Markdown
Contributor Author

@Ethan-Arrowood sorry to ping you again but I'm working on multiple pr now so would you review this code and see whether merge this ? Or to tell me something to change or modify ?Thanks!

@Archkon

Archkon commented Jul 23, 2026

Copy link
Copy Markdown
Contributor Author

@Ethan-Arrowood Sorry to keep ping and may I ask you to review this again ? As the what the issue said #64390

this bug also affects other users and has significant impact on the reliability of request-abort handling"

@Ethan-Arrowood Ethan-Arrowood left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sorry for the delay! GH notifications are a mess these days. This is great work.

@Ethan-Arrowood Ethan-Arrowood added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 23, 2026
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 23, 2026
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@Archkon

Archkon commented Jul 24, 2026

Copy link
Copy Markdown
Contributor Author

@Ethan-Arrowood Could you take a look at these failed tests ?

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@Ethan-Arrowood

Copy link
Copy Markdown
Contributor

All flake; I have it retrying now. I'll watch this today and see if we can get it to land for you.

@Archkon

Archkon commented Jul 26, 2026

Copy link
Copy Markdown
Contributor Author

@Ethan-Arrowood Sorry, May I ask whether you was occupied with something :-) ?

@Ethan-Arrowood

Copy link
Copy Markdown
Contributor

Simply wasn't at my computer any longer. Will you rebase on main ? It may make things more reliable.

IncomingMessage 'close' is emitted when request handling completes, not
only when the underlying connection is closed. Using that event to abort
IncomingMessage.signal makes the signal abort after a request body is
read normally.

Track the underlying socket close instead, and detach the listener when
the request or response completes normally. This keeps the signal from
being aborted by normal stream completion or later keep-alive socket
closure, while still aborting it when the connection closes during the
active request lifecycle.

Signed-off-by: Archkon <180910180+Archkon@users.noreply.github.com>
@Ethan-Arrowood Ethan-Arrowood added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 27, 2026
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 27, 2026
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@Ethan-Arrowood Ethan-Arrowood added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 27, 2026
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 27, 2026
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@Archkon

Archkon commented Jul 28, 2026

Copy link
Copy Markdown
Contributor Author

@Ethan-Arrowood Could you just rerun the failed github action check? I believe this happened due to flaky test

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@Ethan-Arrowood

Copy link
Copy Markdown
Contributor

Yes, I just retried it now. Sorry this is taking some time. CI seemed to have some issues yesterday. I'm also trying to land a PR of my own and its annoyingly taking some time.

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@Ethan-Arrowood

Copy link
Copy Markdown
Contributor

If this retry fails I'll do a rebuild instead. Good news, CI seems happier today. I was able to merge my own PR moments ago. Will continue monitoring yours and get it landed 😄

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

Labels

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.

req.signal.aborted becomes true after request body parse / stream close in Node 24.18.0

4 participants