quic: fixup quic stream variable chunk len#63230
Conversation
jasnell
added
the
quic
|
Review requested:
|
nodejs-github-bot
added
c++
jasnell
changed the title
Signed-off-by: James M Snell <jasnell@gmail.com>
jasnell
force-pushed
the
jasnell/quic-issue-63216
branch
from
a560a10 to
4ade24f
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #63230 +/- ##
=======================================
Coverage 90.04% 90.04%
=======================================
Files 713 713
Lines 224950 224961 +11
Branches 42530 42527 -3
=======================================
+ Hits 202548 202559 +11
+ Misses 14188 14180 -8
- Partials 8214 8222 +8
🚀 New features to boost your workflow:
|
This comment was marked as outdated.
This comment was marked as outdated.
| // while(!writeSync) { dp(); await } loop would spin | ||
| // synchronously and starve the event loop. | ||
| if (len > stream.#state.writeDesiredSize) { | ||
| drainWakeup ??= PromiseWithResolvers(); |
There was a problem hiding this comment.
This will still stall the connection if it ever happens when there's no other writes or acks pending.
This line will set drainWakeup, which blocks all future writes until unset, and it's only ever unset on completed writes or received acks. For the very first write, or any later write if there's no pending acks, it'll never get unset. Ditto for writeev below.
Mainly happens if you're writing chunks larger than highWaterMark, but I think it can happen for smaller cases too depending on QUIC window behaviour.
| // classic 0-to-positive transition and the case where writeDesiredSize | ||
| // was already positive but too small for the next chunk. The JS drain | ||
| // handler is a no-op when no drainWakeup is pending, so the extra | ||
| // callbacks when nobody is waiting are harmless. |
There was a problem hiding this comment.
Nit: 'harmless' isn't totally true - we still do some non-trivial work to jump into JS and back here every time this fires, and this changes makes that happen very frequently (basically every ack). Not critical, but it'd be easy to avoid this call when we know we don't have a pending drain, might be a nice optimization later.
3 participants
Fixes: #63216