quic: buffer stalls, as maxdata updates do not triggern an update of writeDesired sizes

[martenrichter]

ngtcp2 has no callback to inform us about an arrived maxdata frame.
If the buffering is only external and not within ngtcp2, this can cause a stall.
The equivalent for maxstreamdata is already addressed in #64768 , as here ngtcp2 provides a callback.

Here is a reproduction code, based on the test of @pimterry for the PR of maxstreamdata:

// Flags: --experimental-quic --experimental-stream-iter --no-warnings

// Test: Quic maxdata updates on http/3
// Client sends a body that precisely fills the session window size,
// and verifies that it is data transfer is not stalled.

import { hasQuic, skip } from '../common/index.mjs';
import { readFile } from 'node:fs/promises';
import { setTimeout as sleep } from 'node:timers/promises';

if (!hasQuic) {
  skip('QUIC is not enabled');
}
const { listen, connect } = await import('node:quic');
const { createPrivateKey } = await import('node:crypto');
const { drainableProtocol } = await import('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: 
// considers all framing including some initial session capsules
const BODY = WINDOW - 38;

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

const endpoint = await listen((session) => {
  session.onstream = async (stream) => {
    await serverMayRead;
    // eslint-disable-next-line no-unused-vars
    for await (const _ of stream) { /* reading extends the window */ }
  };
}, {
  sni: { '*': { keys: [key], certs: [cert] } },
  transportParams: {
    initialMaxStreamDataBidiRemote: 1024 * 1024,  // make sure maxstreamdata does not block
    initialMaxData: WINDOW,
  },
  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);

const watchdog = setTimeout(() => {
  console.error('STALLED: no drain after MAX_STREAM_DATA');
  process.exit(1);
}, 5000);
letServerRead();                 // Extend the window, with no ack attached
await writer[drainableProtocol]();

clearTimeout(watchdog);
process.exit(0);

This is the ngtcp2 issue:
ngtcp2/ngtcp2#2243 .

Or is there another way without a ngtcp2 callback? (@pimterry @jasnell )

[pimterry]

Did some digging, and I agree we have the same stall today for MAX_DATA.

I think there's a broader issue here: we're aiming for a callback-driven model and that's not what ngtcp2 is consistently providing. There are callbacks, but not for everything - I think they're partly as an optimization/convenience and the design is expecting us to proactively check levels instead (e.g. read ngtcp2_conn_get_max_data_left at the relevant points). We could do that. Even without a max data callback, we could just check limits at the end of each receive loop, and update & act on the result then without worrying about specific frame details. Doing it at the end of the loop also coalesces multiple updates in a single receive loop.

All of this is really cached state invalidation. From what I can see write_desired_size & the streams' blocked state (both Stream::Unschedule and nghttp3_conn_block_stream) are effectively caching a state that's dependent on:

• Stream flow control (MAX_STREAM_DATA)
• Connection flow control (MAX_DATA)
• Our stream budget (set from JS)
• Currently queued bytes (up on writes, down on acks)
• Stream pending state (we jump from buffer limit to buffer + FC windows when the stream stops pending via MAX_STREAMS)
• Stream shutdown/reset state (received RESET_STREAM, or local STOP_SENDING)

I think that's everything? Any of those can change the correct value for write_desired_size and imply blocking/unblocking a stream. Would be very interesting to find ways to test each in isolation.

We have to update this derived state when any of those inputs change. I don't have a quick fix, but it would be nice to reorganize this a bit to do an update at the right boundaries automatically, instead of tactically patching each missing possible trigger one by one, and hoping we don't miss any future cases too. A lot of this could probably be handled generically by doing a read & update of each at the end of the receive loop, and moving us away from the callback approach? Or at least a dcheck backstop to make it easier to detect missed cases.

Would be interesting to explore and test tradeoffs (especially: can we make that efficient). Might imply a broader state check, but would avoid some deferred emit logic since we can do it after the receive loop, I'm not sure what the net impact would be.

[martenrichter]

This cache-invalidating picture is a good way to think about it. I think there is a third option: we can make it not depend on the internal state.
So if we just check for backpressure on the external buffers not yet committed to with its own budget, then we would not run into trouble (e.g. again stream window size). But it is less accurate and can double the memory footprint.