quic: add option waitUntilAvailable to creating streams - #65331
martenrichter wants to merge 9 commits into
Conversation
|
Review requested:
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #65331 +/- ##
==========================================
- Coverage 90.36% 90.36% -0.01%
==========================================
Files 790 792 +2
Lines 274292 275325 +1033
Branches 52510 52757 +247
==========================================
+ Hits 247874 248788 +914
- Misses 16901 16964 +63
- Partials 9517 9573 +56
🚀 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.
There was a problem hiding this comment.
I have tried to fix the remaining stuff using the github web ui....
|
@jasnell I just ping this, was not on the dashboard in the call. It should be almost ready, I will test it at the weekend. (Btw. where is the quic dashboard, I do not find it in this repo) |
|
Added to the project board! will try to do a follow up review on monday |
|
@jasnell and where can I find the project board ? |
|
It's at https://github.com/orgs/nodejs/projects/17/ @martenrichter, I think it's marked as private so you won't be able to see it though, that's probably the issue. @jasnell is that intentional? Could just be public. I can't change it, it says org owners only. |
|
Yes, I get a 404 . |
|
I am currently testing some tests, that verify, if stream limits are respected. But it does not seem to be the case. I will investigate and report back if this PR is affected or if the problem is with another part of the code. |
|
Well, interesting If a stream, due to stream limits, is not immediately created, the |
|
Correction: you do not seem to get a pending stream through |
|
The design intent here was to return the |
|
Ok, I agree, a separate PR is almost ready, just the last bugs needs to be found. |
|
Added a separate PR #65862 for the other issue. |
|
Ok, so we're going to make If we do that, Maybe we should just expose the remaining budget directly, so cases who care about the budget before they open the stream can precisely check it instead? More flexible, and easy to do: just exposing |
|
Well the name Exposing the budget would be an alternative. But we can't put it in state, as we would also have a caching issue. May be we just rename it to |
|
I think the performance cost is roughly the same in either case (querying the budget or passing Different is mostly API UX, so I guess it depends how people will use this. Exposing the budget gives you strictly more information (maybe you want to create 2 streams, and know that you have enough budget for both beforehand) but it's slightly less convenient to check it and throw yourself, instead of just passing |
|
I am also not sure. These last two issues just came up, when I tried to implement W3C webtransport. |
|
Btw. I did not remove the async yet. I just noted that it is currently not used. There may be some benefits to keep it, for example, we could use async commands in the future. |
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>
|
Sorry for the lack of progress; I was away for the last few weeks. I have been back and rebasing my PRs since yesterday, which has taken some time due to the refactoring. |
|
Yeah I spotted an issue earlier this week that I think was introduced by the byte budget changes. Haven't had the opportunity to track it down yet |
|
Well, then I will start with tracking it down now; the test in this PR seems to be susceptible to it. |
|
Currently my best bet is the comment: in http3.cc, but we will see. |
|
Got it, the capsule for extending the stream limit is not sent. Next step: I figure out why. |
4684e2a to
f450014
Compare
|
Found it, it was just a badly rebased test. |
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