Skip to content

Conversation

@martinslota
Copy link

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
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 4 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
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.

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

3 participants