Skip to content

Conversation

@martenrichter
Copy link
Contributor

After the change the body parameter for all
created QuicStreams can take a ReadableStream
of Uint8Array, Arraybuffers etc..

We introduce a DataQueueFeeder class that may be
also used for other related mechanisms.

A locally created unidirectional Stream can not
have a reader. Therefore, the interface is changed
to handle this case. (undercovered when writing
the tests).

Furthermore, a ResumeStream must be added after
AddStream in QuicSession, as the ResumeStream
beforehand triggered with set_outbound is a no-op,
as Stream was not added to Session beforehand.

Fixes: #60234

@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 related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Oct 12, 2025
@martenrichter
Copy link
Contributor Author

@jasnell This try to add support for streams for outgoing data. I hope the PR is helpful and does not waste too much time. I have contributed long time ago 1-2 patches, but I am not that fluent with the node.js codebase.

@codecov
Copy link

codecov bot commented Oct 12, 2025

Codecov Report

❌ Patch coverage is 80.86957% with 44 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.83%. Comparing base (c2a4acb) to head (b22690d).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/dataqueue/queue.cc 51.85% 24 Missing and 2 partials ⚠️
src/quic/streams.cc 85.14% 3 Missing and 12 partials ⚠️
src/quic/streams.h 88.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #60237      +/-   ##
==========================================
+ Coverage   88.57%   88.83%   +0.26%     
==========================================
  Files         704      704              
  Lines      207863   208084     +221     
  Branches    40047    40142      +95     
==========================================
+ Hits       184105   184848     +743     
+ Misses      15789    15168     -621     
- Partials     7969     8068      +99     
Files with missing lines Coverage Δ
lib/internal/quic/quic.js 100.00% <100.00%> (ø)
src/quic/bindingdata.h 33.33% <ø> (ø)
src/quic/quic.cc 100.00% <100.00%> (ø)
src/quic/session.cc 40.79% <100.00%> (+11.70%) ⬆️
src/quic/session.h 43.75% <ø> (ø)
src/quic/streams.h 75.86% <88.00%> (+75.86%) ⬆️
src/quic/streams.cc 45.37% <85.14%> (+42.16%) ⬆️
src/dataqueue/queue.cc 67.25% <51.85%> (+2.62%) ⬆️

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

@martenrichter
Copy link
Contributor Author

There is a crash on some platforms. TODO for next weekend.

@trivikr trivikr added the quic Issues and PRs related to the QUIC implementation / HTTP/3. label Oct 14, 2025
@martenrichter
Copy link
Contributor Author

Locally, I do not have any crashes. Please note, that some of the commits fix errors present in the actual implementation.
I did not make separate PR as these were uncovered by the test included in the commit.
Most of them are annoying race conditions or problems with the lifetime of the objects (use after free!), though I do not see security implications as at the current stage, I do not assume a production usage of quic.

@martenrichter
Copy link
Contributor Author

It seems, that Linux arm is crashing (or is flaky). So probably, I have to go to a Linux container, next weekend) or something else to figure out, what is happening there. (But it may be a problem not introduced by this PR).
(I hope it is a general flakyness on Linux and not ARM specific, or specific on the timing of the runner).

@jasnell
Copy link
Member

jasnell commented Oct 20, 2025

Btw I should have time to look at this PR this upcoming weekend

@martenrichter
Copy link
Contributor Author

martenrichter commented Oct 25, 2025

Btw I should have time to look at this PR this upcoming weekend

This would be great. I am currently trying to reproduce the crash in a non arm container, no luck so far. I will try to use a github code space for arm, next. (Edit: no arm codespaces, but may be the x86 gives a different timing).

@martenrichter
Copy link
Contributor Author

martenrichter commented Oct 26, 2025

This time only unrelated MacOS failures. And I could not reproduce the previous arm faliures locally, may be the rebase
has fixed it.

@martenrichter
Copy link
Contributor Author

Now, Code coverage is increased, and no failing tests.

@martenrichter
Copy link
Contributor Author

The newly added test is actually flaky. Sending out data stalls depending on timing. I have to investigate, why SendPendingData is not called any more.

@martenrichter
Copy link
Contributor Author

The new test is failing as it uncovered a yet-to-be-determined race condition.
What I see is that after submitting 60000, 12, and 50000 bytes, the last submission stalls. On the receive side, 58881 bytes are received. On the send side, it is unclear whether it is flow control or if the sending is just not rescheduled in the Application object. Ideas welcome for the cause, I will continue next weekend. (Review can still start, bug fixing is unrelated.).

Note that I also found another issue (use after free) during debugging related to using next bound to Session and Stream object that are not valid anymore; this should be fixed. It could be a general pitfall when using Bob.

After the change the body parameter for all
created QuicStreams can take a ReadableStream
of Uint8Array, Arraybuffers etc..

We introduce a DataQueueFeeder class, that may be
also used for other related mechanisms.

A locally created unidirectional Stream can not
have a reader. Therefore the interface is changed
to handle this case. (undercovered when writing
 the tests).

Furthermore, a ResumeStream must be added After
AddStream in QuicSession, as the ResumeStream
beforehand triggered with set_outbound is a no-op,
as Stream was not added to Session beforehand.

Fixes: nodejs#60234
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 related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. quic Issues and PRs related to the QUIC implementation / HTTP/3.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

quic: body is lacking support for streaming

4 participants