Skip to content

http: fix keep-alive socket reuse race in requestOnFinish#61710

Closed
martinslota wants to merge 5 commits intonodejs:mainfrom
martinslota:fix-issue-60001
Closed

http: fix keep-alive socket reuse race in requestOnFinish#61710
martinslota wants to merge 5 commits intonodejs:mainfrom
martinslota:fix-issue-60001

Conversation

@martinslota
Copy link
Contributor

When the response ends before the request's 'finish' event fires, responseOnEnd() and requestOnFinish() can both call responseKeepAlive(), corrupting the socket that the agent may have already handed to another request.

This PR adds a !req.destroyed guard to requestOnFinish() so the second call is skipped when the socket has already been released.

Fixes #60001.

Relates to aws/aws-sdk-js-v3#6426.

@nodejs-github-bot
Copy link
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 Feb 6, 2026
@martinslota
Copy link
Contributor Author

The fix can also be backported to Node.js versions 19 and upwards.

When the HTTP response ends before the request's 'finish' event fires,
responseOnEnd() and requestOnFinish() can both call responseKeepAlive(),
corrupting the socket that the agent may have already handed to another
request.

This commit adds a !req.destroyed guard to requestOnFinish() so the
second call is skipped when the socket has already been released.

Fixes: nodejs#60001
@codecov
Copy link

codecov bot commented Feb 9, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.73%. Comparing base (911b158) to head (52c3851).
⚠️ Report is 43 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #61710      +/-   ##
==========================================
- Coverage   89.74%   89.73%   -0.01%     
==========================================
  Files         675      675              
  Lines      204601   204605       +4     
  Branches    39319    39317       -2     
==========================================
- Hits       183611   183610       -1     
+ Misses      13275    13270       -5     
- Partials     7715     7725      +10     
Files with missing lines Coverage Δ
lib/_http_client.js 97.37% <100.00%> (+<0.01%) ⬆️

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

Copy link
Member

@pimterry pimterry left a comment

Choose a reason for hiding this comment

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

LGTM 👍 Nice work on a tricky edge case @martinslota

@pimterry pimterry added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Feb 9, 2026
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 9, 2026
@nodejs-github-bot
Copy link
Collaborator

@martinslota
Copy link
Contributor Author

Looks like some checks have failed - some of them related to an event loop delay test (example):

duration_ms: 7221.425
exitcode: 1
severity: fail
stack: |-
  node:internal/assert/utils:146
    throw error;
    ^

  AssertionError [ERR_ASSERTION]: The expression evaluated to a falsy value:

    assert(histogram.min > 0)

      at Immediate.<anonymous> (/home/iojs/build/workspace/node-test-commit-linux-containered/test/sequential/test-performance-eventloopdelay.js:74:9)
      at Immediate._onImmediate (/home/iojs/build/workspace/node-test-commit-linux-containered/test/common/index.js:507:15)
      at process.processImmediate (node:internal/timers:504:21) {
    generatedMessage: true,
    code: 'ERR_ASSERTION',
    actual: false,
    expected: true,
    operator: '==',
    diff: 'simple'
  }

  Node.js v26.0.0-pre

I'm not really sure how to fix that.

@nodejs-github-bot
Copy link
Collaborator

@pimterry
Copy link
Member

Not to worry I strongly suspect that's a flaky test @martinslota, I've re-run the failures.

@nodejs-github-bot
Copy link
Collaborator

pimterry pushed a commit that referenced this pull request Feb 13, 2026
When the HTTP response ends before the request's 'finish' event fires,
responseOnEnd() and requestOnFinish() can both call responseKeepAlive(),
corrupting the socket that the agent may have already handed to another
request.

This commit adds a !req.destroyed guard to requestOnFinish() so the
second call is skipped when the socket has already been released.

Fixes: #60001
PR-URL: #61710
Reviewed-By: Tim Perry <pimterry@gmail.com>
@pimterry
Copy link
Member

Landed in 37ff1ea

@pimterry pimterry closed this Feb 13, 2026
@martinslota martinslota deleted the fix-issue-60001 branch February 13, 2026 17:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author ready PRs that have at least one approval, no pending requests for changes, and a CI started. 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.

Certain kinds of HTTPS requests hang due to a race condition around reused sockets

4 participants