Skip to content

quic: retain readers for readable streams - #66407

Closed
GiHoon1123 wants to merge 1 commit into
nodejs:mainfrom
GiHoon1123:fix/quic-unidirectional-reader
Closed

GiHoon1123 wants to merge 1 commit into
nodejs:mainfrom
GiHoon1123:fix/quic-unidirectional-reader

Conversation

@GiHoon1123

@GiHoon1123 GiHoon1123 commented Sep 30, 2026 •

Copy link
Copy Markdown

Fixes: #66347

When a peer-initiated unidirectional stream sent data and FIN before the consumer performed
its first async iterator pull, the native stream could be cleaned up before the reader was
created. The buffered data was then no longer available to the JavaScript stream.

Create the reader when a readable QuicStream is constructed so the underlying data queue
remains available until the consumer pulls from the iterator. Local unidirectional streams
remain unreadable and do not allocate a reader.

This means every readable stream allocates a reader whether or not it's ever read, instead
of only on first pull. #66347 asked whether lazy allocation was the right call for this;
eager allocation is the answer this PR takes to that question.

Added a regression test covering data received before the first iterator pull.

Tests:

  • make lint-js
  • QUIC-enabled Node build
  • Related QUIC stream tests

Create readers when readable QuicStream instances are constructed so data received before the first async iterator pull survives native stream cleanup. Add a regression test for a peer-initiated unidirectional stream that sends data and FIN before the reader is pulled.

Assisted-by: Codex
Signed-off-by: GiHoon1123 <rlaejrqo465@naver.com>
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/quic

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. quic Issues and PRs related to the QUIC transport implementation. labels Sep 30, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Welcome to Node.js, and thank you for your first contribution!

Before review, please take a moment to read:

Please make sure every commit is signed off. For a first pull request, GitHub Actions require collaborator approval and Jenkins CI must be started by a collaborator or triager, so an initial wait is normal.

@MikeMcC399

Copy link
Copy Markdown
Contributor

The recommendations in:

advise you should tackle only one issue at a time and that you should not open any new PRs until your first PR has been approved.

@GiHoon1123

Copy link
Copy Markdown
Author

Sorry about that - didn't follow the one-issue-at-a-time guideline. Closing this for now and focusing on #64627 first, will reopen once that's merged.

@GiHoon1123 GiHoon1123 closed this Sep 30, 2026
@MikeMcC399

Copy link
Copy Markdown
Contributor

The problem with multiple PRs for first-time contributors is that every step need to be manually approved by a team member, which creates a high workload and slows down the progression of the PRs.

@codecov

codecov Bot commented Sep 30, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.36%. Comparing base (f71d644) to head (2526c29).
⚠️ Report is 20 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #66407   +/-   ##
=======================================
  Coverage   90.36%   90.36%           
=======================================
  Files         792      792           
  Lines      275564   275584   +20     
  Branches    52832    52843   +11     
=======================================
+ Hits       249016   249042   +26     
+ Misses      16965    16957    -8     
- Partials     9583     9585    +2     
Files with missing lines Coverage Δ
lib/internal/quic/quic.js 100.00% <100.00%> (ø)

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

needs-ci PRs that need a full CI run. quic Issues and PRs related to the QUIC transport implementation.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

quic: reader is lazily allocated in QuicStream, an incoming unidi stream with fin will loose its data

3 participants