Skip to content

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
Merged

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

Conversation

@Archkon

@Archkon Archkon commented Jul 4, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Fixes: #64272

@nodejs-github-bot

nodejs-github-bot commented Jul 4, 2026

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 4, 2026
@Archkon
http: do not emit socket errors after complete response
b04af3c 
Signed-off-by: Archkon <180910180+Archkon@users.noreply.github.com>
@mcollina mcollina added the baking-for-lts PRs that need to wait before landing in a LTS release. label Jul 5, 2026
@mcollina

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
mcollina approved these changes Jul 5, 2026
View reviewed changes

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

lgtm

@mcollina mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 5, 2026
@github-actions 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

nodejs-github-bot commented Jul 5, 2026

Copy link
Copy Markdown
Collaborator

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

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

Archkon commented Jul 12, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@mcollina @jasnell This pr would open for one week, should we ship this to main branch and not let the regession behavior which the issue describled last for long time?

@mcollina mcollina mentioned this pull request Jul 12, 2026
Closed
mcollina
mcollina approved these changes Jul 12, 2026
View reviewed changes

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

lgtm

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

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 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
nodejs-github-bot merged commit 17163ea into nodejs:main Jul 12, 2026
74 checks passed
@nodejs-github-bot

nodejs-github-bot commented Jul 12, 2026

Copy link
Copy Markdown
Collaborator

Landed in 17163ea

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

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

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.

@Archkon

Archkon commented Jul 14, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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.

I do check res.completed in my first version of commit but that would cause some tests failed which maybe associated to macos platform difference so I only keep check whether only res exists and I don't remember the details completely now but thanks for continuing to accomplish this

@pimterry pimterry mentioned this pull request Jul 14, 2026
Open
@mcollina

mcollina commented Jul 14, 2026

Copy link
Copy Markdown
Member

@lpinca should we revert?

@mcollina 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 mcollina mentioned this pull request Jul 15, 2026
Merged
Archkon added a commit to Archkon/node that referenced this pull request Jul 17, 2026
@Archkon
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 Archkon mentioned this pull request Jul 17, 2026
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lpinca lpinca lpinca left review comments
@mcollina mcollina mcollina approved these changes
@jasnell 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

@Archkon @nodejs-github-bot @mcollina @lpinca @pimterry @jasnell