quic: add option waitUntilAvailable to creating streams - #65331
quic: add option waitUntilAvailable to creating streams#65331martenrichter wants to merge 3 commits into
Conversation
We add an option to createBidirectionalStream and createUnidirectionalStream to fail immediately, if the flow control's stream budget does not allow stream creation. The behavior matches W3C webtransport's behavior. Fixes nodejs#65321 Signed-off-by: Marten Richter <marten.richter@freenet.de>
|
Review requested:
|
nodejs-github-bot
added
c++
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #65331 +/- ##
==========================================
- Coverage 90.30% 90.11% -0.20%
==========================================
Files 751 752 +1
Lines 250235 251570 +1335
Branches 47305 47266 -39
==========================================
+ Hits 225987 226691 +704
- Misses 15619 16238 +619
- Partials 8629 8641 +12
🚀 New features to boost your workflow:
|
| }); | ||
|
|
||
| // Second stream is created but queued as pending because the | ||
| try { |
| body: encoder.encode('stream 2'), | ||
| const s3 = await clientSession.createBidirectionalStream({ | ||
| body: encoder.encode('stream 3'), | ||
| waitUntilAvailable: true |
| waitUntilAvailable: false | ||
| }); | ||
| await s4.closed; | ||
| await allDone.promise; |
There was a problem hiding this comment.
await Promise.all([s4.closed, allDone.promise]);
| @@ -42,6 +42,7 @@ const s1 = await clientSession.createUnidirectionalStream({ | |||
| // Second uni stream is pending (limit = 1). | |||
| const s2 = await clientSession.createUnidirectionalStream({ | |||
| body: encoder.encode('uni 2'), | |||
| waitUntilAvailable: true, | |||
There was a problem hiding this comment.
If waitUntilAvailable: true is the default we don't need to change all these, right?
There was a problem hiding this comment.
No you suggested to default it to false.
There was a problem hiding this comment.
Which matches W3C
There was a problem hiding this comment.
Sorry, that was a mistake on my part. Since it returns a promise for the stream, I think it's more ergonomic to wait by default. The Web Transport API impl can easily pass false but I think what most users would likely typically expect is that the promise resolves with the stream when the stream is actually available.
There was a problem hiding this comment.
Ok, I will change this.
| writing more. **Default:** `65536` (64 KB). | ||
| * `waitUntilAvailable` {boolean} When true the promise will wait until flow | ||
| control will allow to open the stream. If set to false, the function | ||
| will fail synchronously, if flow control will not allow to open the stream |
There was a problem hiding this comment.
These methods return a promise. While it may fail synchronously internally, it should return a rejected promise and not throw synchronously.
There was a problem hiding this comment.
Do you have the C++ command in mind?
There was a problem hiding this comment.
Or will it be automatically like this through the js side and I do not have to change anything on the c++ side?
| control will allow to open the stream. If set to false, the function | ||
| will fail synchronously, if flow control will not allow to open the stream | ||
| immediately. | ||
| **Default:** `false` |
There was a problem hiding this comment.
Default should be true I think... just from an ergonomics point of view.
There was a problem hiding this comment.
Your initial suggestion was in the other direction. W3C WT uses also false.
P.S: I have to leave my dev machine for a while, so I might not update this for a while.
3 participants
We add an option to createBidirectionalStream and
createUnidirectionalStream to fail immediately,
if the flow control's stream budget does not
allow stream creation.
The behavior matches W3C webtransport's behavior.
Fixes #65321