Skip to content

quic: add promise to QuicStream for pending strms - #65862

Open
martenrichter wants to merge 5 commits into
nodejs:mainfrom
martenrichter:addreadytoquicstream
Open

quic: add promise to QuicStream for pending strms#65862
martenrichter wants to merge 5 commits into
nodejs:mainfrom
martenrichter:addreadytoquicstream

Conversation

@martenrichter

Copy link
Copy Markdown
Contributor

before this PR, it was necessary to poll, if a
stream can not be created immediately due to
flow control.
This PR adds a promise to QuicStream, that fulfills, when a stream is available and ready.

before this PR, it was necessary to poll, if a
stream can not be created immediately due to
flow control.
This PR adds a promise to QuicStream, that fulfills,
when a stream is available and ready.

Signed-off-by: Marten Richter <marten.richter@freenet.de>
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/quic

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs involving general changes in the lib/ or src/ directories. needs-ci PRs that need a full CI run. labels Sep 6, 2026
@martenrichter

Copy link
Copy Markdown
Contributor Author

@jasnell @pimterry that is the separate PR for a promise for pending streams

@bjohansebas bjohansebas added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Sep 6, 2026
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Sep 6, 2026
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@codecov

codecov Bot commented Sep 6, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.17%. Comparing base (ad67159) to head (a31c748).
⚠️ Report is 64 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #65862      +/-   ##
==========================================
+ Coverage   90.14%   90.17%   +0.02%     
==========================================
  Files         769      771       +2     
  Lines      262968   265132    +2164     
  Branches    50060    50352     +292     
==========================================
+ Hits       237057   239076    +2019     
- Misses      16932    17019      +87     
- Partials     8979     9037      +58     
Files with missing lines Coverage Δ
lib/internal/quic/quic.js 100.00% <100.00%> (ø)
lib/internal/quic/symbols.js 100.00% <100.00%> (ø)

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

Comment thread doc/api/quic.md Outdated
added: v23.8.0
-->

### `stream.ready`

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Imo stream.opened would be a slightly clearer API, to match session.opened and stream.stats.opened_at.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ok, I have changed it. (Will push soon). readycame from WebStream semantics.

Comment thread lib/internal/quic/quic.js Outdated
@trivikr trivikr added the quic Issues and PRs related to the QUIC transport implementation. label Sep 7, 2026
@jasnell
jasnell requested a review from pimterry September 8, 2026 17:49

@pimterry pimterry left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for the fixes @martenrichter. Code changes all look good to me.

In the docs though the name of the field is wrong, and there's now a conflict here so this'll need a rebase. Otherwise LGTM.

Comment thread doc/api/quic.md
-->

### `stream.ready`
### `stream.open`

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Docs say open, but the actual field is opened.

@jasnell

jasnell commented Sep 8, 2026

Copy link
Copy Markdown
Member

@martenrichter ... this will need to be rebased

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

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs involving general changes in the lib/ or src/ directories. 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.

6 participants