Skip to content

stream: handle falsy push writer fail reasons#63569

Open
trivikr wants to merge 1 commit into
nodejs:mainfrom
trivikr:stream-iter-writer-fail-pending
Open

stream: handle falsy push writer fail reasons#63569
trivikr wants to merge 1 commit into
nodejs:mainfrom
trivikr:stream-iter-writer-fail-pending

Conversation

@trivikr
Copy link
Copy Markdown
Member

@trivikr trivikr commented May 25, 2026

This fixes stream/iter push writer reads after writer.fail() is called
with an explicit falsy reason.

Reads now use the writer state instead of the truthiness of the stored error,
so reasons like 0 and false reject correctly instead of leaving reads
pending.

Fixes: #63568


Assisted-by: openai:gpt-5.5

@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/streams

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. stream Issues and PRs related to the stream subsystem. labels May 25, 2026
@codecov
Copy link
Copy Markdown

codecov Bot commented May 25, 2026

Codecov Report

❌ Patch coverage is 90.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 90.33%. Comparing base (8b88e2c) to head (bdda946).

Files with missing lines Patch % Lines
lib/internal/streams/iter/push.js 90.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #63569      +/-   ##
==========================================
- Coverage   91.95%   90.33%   -1.63%     
==========================================
  Files         379      732     +353     
  Lines      166486   236459   +69973     
  Branches    25452    44534   +19082     
==========================================
+ Hits       153099   213610   +60511     
- Misses      13094    14548    +1454     
- Partials      293     8301    +8008     
Files with missing lines Coverage Δ
lib/internal/streams/iter/push.js 91.63% <90.00%> (+0.05%) ⬆️

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

@trivikr trivikr force-pushed the stream-iter-writer-fail-pending branch 2 times, most recently from 047dd35 to cf08dca Compare May 27, 2026 15:37
@trivikr trivikr added the request-ci Add this label to start a Jenkins CI on a PR. label May 28, 2026
Reject reads based on writer state so explicit falsy fail reasons do
not leave reads pending.

Fixes: nodejs#63568

Signed-off-by: Kamat, Trivikram <16024985+trivikr@users.noreply.github.com>
Assisted-by: openai:gpt-5.5
@trivikr trivikr force-pushed the stream-iter-writer-fail-pending branch from cf08dca to bdda946 Compare May 31, 2026 05:25
Copy link
Copy Markdown
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-ci PRs that need a full CI run. request-ci Add this label to start a Jenkins CI on a PR. stream Issues and PRs related to the stream subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

stream/iter: writer.fail(0) can leave async iterator readers pending forever

3 participants