Skip to content

Prevent unhandled rejections when cancelling an errored stream reader - #22371

Merged
Snuffleupagus merged 1 commit into
mozilla:masterfrom
andrewfader:stream-reader-cancel-errored
Oct 31, 2026
Merged

Snuffleupagus merged 1 commit into
mozilla:masterfrom
andrewfader:stream-reader-cancel-errored

Conversation

@andrewfader

@andrewfader andrewfader commented Oct 28, 2026 •

Copy link
Copy Markdown
Contributor

Fixes #22366

Cancelling a ReadableStreamDefaultReader whose stream has errored, e.g. after a network error, returns a promise rejected with that error, which was never handled. This affects the fetch, worker, and Node.js stream readers.

The new tests in fetch_stream_spec.js and node_stream_spec.js fail on master with an unhandled rejection, and pass with this patch. They don't need any new test files.

Regarding the review of #22306: the same fix is applied to node_stream.js, as well as to worker_stream.js, so the implementations stay in sync.

Comment thread test/pdfs/.gitignore Resolved
Comment thread test/unit/api_spec.js Resolved
Comment thread test/unit/node_stream_spec.js Resolved
@andrewfader
andrewfader force-pushed the stream-reader-cancel-errored branch 2 times, most recently from e91f72b to 6875c96 Compare October 28, 2026 19:23
Comment thread test/unit/node_stream_spec.js Resolved
Comment thread test/unit/node_stream_spec.js Resolved
Comment thread test/unit/node_stream_spec.js Resolved
@andrewfader
andrewfader force-pushed the stream-reader-cancel-errored branch 2 times, most recently from 4dd68f3 to 215e2fa Compare October 29, 2026 18:05
@codecov-commenter

codecov-commenter commented Oct 30, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.58%. Comparing base (22f5df1) to head (743422f).
⚠️ Report is 14 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #22371      +/-   ##
==========================================
- Coverage   90.58%   90.58%   -0.01%     
==========================================
  Files         278      278              
  Lines       68568    68584      +16     
==========================================
+ Hits        62115    62125      +10     
- Misses       6453     6459       +6     
Flag Coverage Δ
browsertest 66.27% <66.66%> (-0.02%) ⬇️
fonttest 8.99% <ø> (ø)
integrationtest 69.60% <33.33%> (+0.05%) ⬆️
unittest 61.27% <66.66%> (+<0.01%) ⬆️
unittestcli 59.01% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 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.

@github-actions

github-actions Bot commented Oct 30, 2026 •

Copy link
Copy Markdown
Contributor

Viewer preview

🗑️ Viewer previews removed.

@codspeed

codspeed Bot commented Oct 30, 2026 •

Copy link
Copy Markdown

Merging this PR will improve performance by 24.42%

⚡ 1 improved benchmark
✅ 87 untouched benchmarks
⏩ 174 skipped benchmarks1

Performance Changes

Benchmark BASE HEAD Efficiency
⚡ getUnicodeRangeFor (bug1473809.pdf) 33.4 ms 26.9 ms +24.42%

Tip

Curious why performance improved? Comment @codspeedbot explain why performance improved on this PR, or directly use the CodSpeed MCP with your agent.


Comparing andrewfader:stream-reader-cancel-errored (743422f) with master (ab3819f)

Open in CodSpeed

Footnotes

  1. 174 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩

Comment thread test/unit/fetch_stream_spec.js Resolved
Comment thread test/unit/node_stream_spec.js Resolved
Comment thread test/unit/node_stream_spec.js Resolved
@Snuffleupagus

Copy link
Copy Markdown
Collaborator

The commits must be squashed.

@andrewfader
andrewfader force-pushed the stream-reader-cancel-errored branch from 4c7ddfe to 9af48f2 Compare October 31, 2026 19:01
@Snuffleupagus

Copy link
Copy Markdown
Collaborator

Hmm, why does the commit now list me as co-author?

@andrewfader

Copy link
Copy Markdown
Contributor Author

Whoops sorry it's because I squashed the auto-suggested review commits. I'll --reset-author.

Cancelling a `ReadableStreamDefaultReader` whose stream has errored, e.g. after a network error, returns a promise rejected with that error, which was never handled. This affects the fetch, worker, and Node.js stream readers.
@andrewfader
andrewfader force-pushed the stream-reader-cancel-errored branch from 9af48f2 to 743422f Compare October 31, 2026 20:26
@andrewfader

Copy link
Copy Markdown
Contributor Author

Sorry @Snuffleupagus, force-pushed a clean commit with just me as author.

@github-actions

github-actions Bot commented Oct 31, 2026 •

Copy link
Copy Markdown
Contributor

Reference tests

Results for 743422f (run, references from mozilla/pdf.js.refs@d16403bcaa).

Platform Status Tests Total runtime Errors Different FBF No reference Report
Linux ⚠️ 1413 12m 21s 0 10 0 0 view differences
Windows ⚠️ 1413 14m 2s 0 10 0 0 view differences

The report is kept for 30 days and replaced by the next run.

sync-pdfs-for-pdf-js Bot pushed a commit to mozilla/pdf.js.refs that referenced this pull request Oct 31, 2026
@Snuffleupagus
Snuffleupagus merged commit 0a41533 into mozilla:master Oct 31, 2026
28 checks passed
@andrewfader
andrewfader deleted the stream-reader-cancel-errored branch October 31, 2026 21:50

This branch was successfully deployed

1 active deployment
sync_pdfs — 743422f8 Deployed Oct 31, 2026 by Snuffleupagus via request #685
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Unhandled rejection when a document that failed to load after a network error is destroyed

4 participants