nodejs / nodejs/node

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

Open
#64,835 2 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

quic
Dominant language
JavaScript
Stars
122k
Forks
37.3k
Avg merge
4d 2h
Merged PRs (30d)
283

Description

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 https://github.com/nodejs/node/pull/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:
https://github.com/ngtcp2/ngtcp2/issues/2243 .

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

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start by running the supplied QUIC reproduction and inspect the write/drain path used by the session and stream writer. Compare the maxdata behavior with the maxstreamdata fix in pull request 64768 and review ngtcp2 issue 2243; done means the writer drain completes after the server extends the session window without stalling.

Written by the indexing model from the issue text.

Assessment

Tech stack
javascript
Domain
networking
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
45/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.