FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

http: do not emit socket errors after complete response by Archkon · Pull Request #64278 · nodejs/node · GitHub

/ node Public

http: do not emit socket errors after complete response - #64278

Merged
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
Archkon:http
Jul 12, 2026
ConversationCommits (1)ChecksFiles changed
Merged

http: do not emit socket errors after complete response#64278
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
Archkon:http

Conversation

Archkon commented Jul 4, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Fixes: #64272

nodejs-github-bot commented Jul 4, 2026

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/http
  • @nodejs/net

nodejs-github-bot added http Issues or PRs related to the http subsystem. needs-ci PRs that need a full CI run. labels Jul 4, 2026
http: do not emit socket errors after complete response
b04af3c 
Signed-off-by: Archkon <180910180+Archkon@users.noreply.github.com>
mcollina added the baking-for-lts PRs that need to wait before landing in a LTS release. label Jul 5, 2026

mcollina commented Jul 5, 2026

Copy link
Copy Markdown
Member

I fear we need to bake this a bit
or it might cause regressions.

mcollina approved these changes Jul 5, 2026
View reviewed changes

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

lgtm

mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 5, 2026
github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 5, 2026

nodejs-github-bot commented Jul 5, 2026

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/74562/

lpinca reviewed Jul 5, 2026
View reviewed changes
Comment thread lib/_http_client.js
Comment thread lib/_http_client.js
Archkon requested review from jasnell and mcollina July 5, 2026 21:02

This comment was marked as spam.

Sign in to view
mcollina mentioned this pull request Jul 12, 2026
Closed
mcollina approved these changes Jul 12, 2026
View reviewed changes

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

lgtm

mcollina added commit-queue Add this label to land a pull request using GitHub Actions. and removed baking-for-lts PRs that need to wait before landing in a LTS release. labels Jul 12, 2026

mcollina commented Jul 12, 2026

Copy link
Copy Markdown
Member

removed the baking for lts, given the bug is in 24, we might want to have it fixed there

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 12, 2026
nodejs-github-bot merged commit 17163ea into nodejs:main Jul 12, 2026
74 checks passed

nodejs-github-bot commented Jul 12, 2026

Copy link
Copy Markdown
Collaborator

Landed in 17163ea

Archkon deleted the http branch July 12, 2026 11:54

lpinca commented Jul 12, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

@nodejs/http fyi, all socket errors after req.res exists are now swallowed. I think this should be semver-major.

pimterry commented Jul 14, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Agreed @lpinca - this swallows errors for started but incomplete responses in problematic ways.

I think it's an easy fix: we just need to check for res.completed. @mcollina your note before merging in #64272 mentioned this specifically as a concern, is there a reason you decided to leave it & merge instead? Not sure if I'm missing something.

I think we have to to preserve reasonable error handling, I'll open a PR to fix ASAP.

This comment was marked as spam.

Sign in to view
pimterry mentioned this pull request Jul 14, 2026
Open

mcollina commented Jul 14, 2026

Copy link
Copy Markdown
Member

@lpinca should we revert?

mcollina added dont-land-on-v22.x PRs that should not land on the v22.x-staging branch and should not be released in v22.x. dont-land-on-v24.x PRs that should not land on the v24.x-staging branch and should not be released in v24.x. dont-land-on-v26.x PRs that should not land on the v26.x-staging branch and should not be released in v26.x. labels Jul 14, 2026
mcollina mentioned this pull request Jul 15, 2026
Merged
Archkon added a commit to Archkon/node that referenced this pull request Jul 17, 2026
http: defer write errors for pending responses
2818b98 
A transport write error can be delivered before a readable event from
the same poll cycle. Writable error handling then destroys both sides of
the socket before the HTTP parser can consume an already-sent response.

Defer native write errors that do not carry protocol-specific details.
After pending reads run, suppress the error only when the request write
and response parse are both complete. Continue reporting open writes,
truncated responses, user destroy errors, and TLS protocol errors.

Follow-up to: nodejs#64507
Original PR Refs: nodejs#64278
Fixes: nodejs#64272
Refs:nodejs#64511
Refs: libuv/libuv#5196
Refs: nodejs#64507 (comment)
Refs: nodejs#64511 (comment)

Signed-off-by: Archkon <180910180+Archkon@users.noreply.github.com>
Archkon mentioned this pull request Jul 17, 2026
Closed
coderabbitai Bot mentioned this pull request Jul 28, 2026
Merged
11 tasks
Archkon added a commit to Archkon/node that referenced this pull request Aug 4, 2026
http: defer write errors for pending responses
dcbb130 
A transport write error can be delivered before a readable event from
the same poll cycle. Writable error handling then destroys both sides of
the socket before the HTTP parser can consume an already-sent response.

Defer native write errors that do not carry protocol-specific details.
After pending reads run, suppress the error only when the request write
and response parse are both complete. Continue reporting open writes,
truncated responses, user destroy errors, and TLS protocol errors.

Follow-up to: nodejs#64507
Original PR Refs: nodejs#64278
Fixes: nodejs#64272
Refs:nodejs#64511
Refs: libuv/libuv#5196
Refs: nodejs#64507 (comment)
Refs: nodejs#64511 (comment)

Signed-off-by: Archkon <180910180+Archkon@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

lpinca lpinca left review comments
mcollina mcollina approved these changes
jasnell Awaiting requested review from jasnell

Assignees

No one assigned

Labels

dont-land-on-v22.x PRs that should not land on the v22.x-staging branch and should not be released in v22.x. dont-land-on-v24.x PRs that should not land on the v24.x-staging branch and should not be released in v24.x. dont-land-on-v26.x PRs that should not land on the v26.x-staging branch and should not be released in v26.x. http Issues or PRs related to the http subsystem. needs-ci PRs that need a full CI run.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

New ECONNRESET error on http.request for HTTP 413 (Node v24.16.0 regression)

6 participants

Back | FazBrowse Home | New Git URL