GeezerVison

Review requested:

• @nodejs/quic

@jasnell @pimterry can you have a look?

Can you add a test?

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.14%. Comparing base (9024119) to head (ad505ca).
⚠️ Report is 411 commits behind head on main.

@@            Coverage Diff             @@
##             main   #64768      +/-   ##
==========================================
- Coverage   90.15%   90.14%   -0.01%     
==========================================
  Files         743      746       +3     
  Lines      242407   242660     +253     
  Branches    45645    45723      +78     
==========================================
+ Hits       218532   218750     +218     
- Misses      15357    15425      +68     
+ Partials     8518     8485      -33     

Can you add a test?

Not really. I found it in my webtransport test suite. But there it happens only, if I use node.js server and client. If I choose a Quiche client and a Node.js server, or a Node.js client and a Quiche server, it does not surface, as it depends on the particular packet size the particular Quic internals are generating at the low level. But I will later port all of my wt tests over, but therefore wt must be ready. (But this must not mean that these would reliably trigger the issue).

Not really. I found it in my webtransport test suite.

I wanted to understand, so I did some digging. I think the trigger is actually quite clear: UpdateWriteDesiredSize was only called on ack, but the stream window is extended independently of acks. A peer can ack all our data, wait (all data acked but zero window available) and then extend the window size to allow more data later - so we do need to update desired size based on the window update alone 👍. Good find @martenrichter! This is tricky to trigger as it's easily obscured by buffering.

We can repro it reliably with something like this:

import { createPrivateKey } from 'node:crypto';
import { readFile } from 'node:fs/promises';
import { setTimeout as sleep } from 'node:timers/promises';
import { listen, connect } from 'node:quic';
import { drainableProtocol } from 'stream/iter';

const keys = 'test/fixtures/keys';
const key = createPrivateKey(await readFile(`${keys}/agent1-key.pem`));
const cert = await readFile(`${keys}/agent1-cert.pem`);

const WINDOW = 4096;
// Fills the window exactly: HTTP/3 spends 11 of those bytes on framing (8 for
// the HEADERS frame below, 3 for the DATA frame header). The send buffer then
// empties at the same moment the window reaches zero, leaving nothing in
// flight to ack. Any other size leaves bytes queued, and the ack for those
// wakes the writer instead, hiding the bug.
const BODY = WINDOW - 11;

let letServerRead;
const serverMayRead = new Promise((resolve) => { letServerRead = resolve; });

const endpoint = await listen((session) => {
  session.onstream = async (stream) => {
    await serverMayRead;
    for await (const _ of stream) { /* reading extends the window */ }
  };
}, {
  sni: { '*': { keys: [key], certs: [cert] } },
  transportParams: {
    initialMaxStreamDataBidiRemote: WINDOW,
    initialMaxData: 1024 * 1024,
  },
  onheaders() { this.sendHeaders({ ':status': '200' }); },
});

const session = await connect(endpoint.address, {
  servername: 'localhost',
  verifyPeer: 'manual',
});
await session.opened;

// Budget well above the window, so the window is what stops the writer.
const stream = await session.createBidirectionalStream({ budget: 1024 * 1024 });
stream.sendHeaders({
  ':method': 'POST',
  ':path': '/',
  ':scheme': 'https',
  ':authority': 'localhost',
}, { terminal: false });

const writer = stream.writer;
writer.writeSync(new Uint8Array(BODY));

// Long enough for every byte to be acked. The peer acks as data arrives,
// whether or not its application has read any of it, so by now the window is
// exhausted, the send buffer is empty, and no further ACK can arrive.
await sleep(500);
console.log('all acked, window exhausted');

const watchdog = setTimeout(() => {
  console.log('STALLED: no drain after MAX_STREAM_DATA');
  process.exit(1);
}, 5000);

letServerRead();                 // extend the window, with no ack attached
console.log('window extended');
await writer[drainableProtocol]();
console.log('writer drain');

clearTimeout(watchdog);
console.log('done');
process.exit(0);

This creates a server which doesn't read, so it'll ack everything without extending the window. The client writes the exact amount to fill the window but leave nothing in its buffers (otherwise the window update triggers more writes, and then gets acks). Then after a 500ms pause, the server reads and triggers a window update.

Fixed client will trigger a drain and complete OK, bad client will stall forever. We should be able to build a test around that directly.

The fix here covers this for HTTP/3, but doesn't totally fix the issue because we have two independent implementations of this with the current application structure, so we'll need to also separately fix the equivalent issue again for pure QUIC in the default app.

And we may also need it for session window, but I think we also need a callback from ngtcp2 for this.
Btw. that is the reason why there was so little progress from the last 3 weekends, it was horrible to debug (and I had a second error in the client code that was interfering).

@pimterry I am tring to integrate your test. But actually, the bug I was catching was different; it was actually two buffers, one inside the window size and the other going beyond the window size. At least this what I remember from debugging.

Ok, also done for pure quic.
What is missing though, is the same for maxdata.
So I think we should create a separate issue for this with test, but I do not see a fix, as the ngtcp2 callback is missing.
I am refering to this line:

Create a separate issue for maxdata, as I have no clue how to address it currently.
#64835

CI: https://ci.nodejs.org/job/node-test-pull-request/76030/

It looks like this PR is ready to land.

It says: "⚠ Could not retrieve the email or name of the PR author's from user's GitHub profile!"
I wonder what this means. PR worked before for me with node.js .

I wouldn't worry about that. I'll squash and merge these directly.

Landed in 2bfc1e9