Skip to content

http: fix writableFinished and 'finish' after write errors - #64847

Open
pimterry wants to merge 1 commit into
nodejs:mainfrom
pimterry:fix-writableFinished
Open

http: fix writableFinished and 'finish' after write errors#64847
pimterry wants to merge 1 commit into
nodejs:mainfrom
pimterry:fix-writableFinished

Conversation

@pimterry

Copy link
Copy Markdown
Member

On HTTP OutgoingMessages, only emit 'finish' and set writableFinished once all data has actually been flushed successfully, and make end() callbacks report the correct outcome like stream.Writable: called with null on finish, or with the error that prevented the flush.

This changes visible behaviour in some subtle ways, but I think exclusively in a more correct direction, which matches Writable everywhere else and matches the existing docs. Now, if an HTTP write calls end() but fails to actually flush the data, it will fail to finish, and the end() callback behaviour correctly matches. See the 2 tests that need changing here due to subtle changes in ordering & finish behaviour.

It's on the line, but I'd really rather not make this semver major because it's a prerequisite for #64566 fixing #64272. That's a regression on v24 & v26 today, due to changes in how libuv exposes ECONNRESET. To work around that change, we need an accurate signal for writableFinished, and we currently don't have one.

Only emit 'finish' and set writableFinished once all data has actually
been flushed successfully. end() callbacks now report the outcome like
stream.Writable: called with null on finish, or with the error that
prevented the flush.

Signed-off-by: Tim Perry <pimterry@gmail.com>
@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 30, 2026
@codecov

codecov Bot commented Jul 30, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 98.03922% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 90.16%. Comparing base (396aad0) to head (ddbd846).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
lib/_http_outgoing.js 98.03% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #64847      +/-   ##
==========================================
- Coverage   90.17%   90.16%   -0.02%     
==========================================
  Files         746      746              
  Lines      242776   242814      +38     
  Branches    45741    45755      +14     
==========================================
+ Hits       218929   218930       +1     
- Misses      15347    15375      +28     
- Partials     8500     8509       +9     
Files with missing lines Coverage Δ
lib/_http_outgoing.js 97.77% <98.03%> (+0.14%) ⬆️

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

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.

3 participants