-
-
Notifications
You must be signed in to change notification settings - Fork 34.2k
quic: For streams allow ReadableStream as body #60237
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
@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. |
10ccd9f to
d31f2c8
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #60237 +/- ##
==========================================
+ Coverage 88.53% 88.79% +0.26%
==========================================
Files 703 704 +1
Lines 208546 208884 +338
Branches 40217 40339 +122
==========================================
+ Hits 184634 185479 +845
+ Misses 15926 15296 -630
- Partials 7986 8109 +123
🚀 New features to boost your workflow:
|
|
There is a crash on some platforms. TODO for next weekend. |
|
Locally, I do not have any crashes. Please note, that some of the commits fix errors present in the actual implementation. |
|
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). |
|
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). |
edb0f7e to
aa0c968
Compare
|
This time only unrelated MacOS failures. And I could not reproduce the previous arm faliures locally, may be the rebase |
aa0c968 to
9cddce0
Compare
|
Now, Code coverage is increased, and no failing tests. |
|
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. |
|
The new test is failing as it uncovered a yet-to-be-determined race condition. 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. |
7be7d5c to
b22690d
Compare
|
I think I finally found the race condition, causing the test to fail. |
src/quic/streams.cc
Outdated
|
|
||
| namespace node { | ||
|
|
||
| using quic::BindingData; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This using should not be necessary
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I remove it at my local copy and see what happens in the build process.
e172429 to
a566b51
Compare
e2d9dcd to
15d2cc1
Compare
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