Skip to content

fix: replace finished() with writable lifecycle tracking#5001

Draft
trivikr wants to merge 1 commit intonodejs:mainfrom
trivikr:stream-finished
Draft

fix: replace finished() with writable lifecycle tracking#5001
trivikr wants to merge 1 commit intonodejs:mainfrom
trivikr:stream-finished

Conversation

@trivikr
Copy link
Copy Markdown
Member

@trivikr trivikr commented Apr 9, 2026

This relates to...

  • Hot-path listener overhead in lib/api/api-stream.js
  • The existing TODO about avoiding stream.finished() in StreamHandler

Rationale

StreamHandler used stream.finished(res, { readable: false }) to observe completion of the user-provided writable. That helper is broader than this path needs and registers extra listeners, which adds avoidable overhead on a hot path.

This change narrows lifecycle tracking to the writable side only while preserving the behavior stream() depends on for completion, writable errors, and premature close handling.

Changes

  • Replaced stream.finished() usage in StreamHandler with a small writable lifecycle helper that listens only for finish, error, and close
  • Preserved premature close behavior by reporting ERR_STREAM_PREMATURE_CLOSE when the writable closes before finishing
  • Preserved existing error propagation by aborting the request with the writable error
  • Avoided re-destroying the writable with the same error after an 'error' event, which keeps existing error handling semantics intact
  • Added regression coverage for premature writable close without an explicit error
  • Added regression coverage for a factory returning an already-finished writable

Features

N/A

Bug Fixes

  • Reduced listener registration overhead in stream() response handling
  • Kept stream() callback and abort behavior consistent for writable finish, error, and premature close edge cases

Breaking Changes and Deprecations

  • None

Status

Posting as draft as this fix was generated by AI, and I don't fully understand the fix.
Maintainers can consider closing it and/or cherry-picking to a new PR.

Signed-off-by: Kamat, Trivikram <16024985+trivikr@users.noreply.github.com>
@codecov-commenter
Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 90.19608% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 92.92%. Comparing base (a434502) to head (ec6c073).

Files with missing lines Patch % Lines
lib/api/api-stream.js 90.19% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5001      +/-   ##
==========================================
- Coverage   92.93%   92.92%   -0.02%     
==========================================
  Files         110      110              
  Lines       35735    35781      +46     
==========================================
+ Hits        33210    33249      +39     
- Misses       2525     2532       +7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 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

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants