Event memory leak for 'error' on session during h2 requests

[simonkberg]

Describe the bug

Event memory leak during h2 requests due to 'error' events not being properly cleaned up on the session during streams.

To Reproduce

I haven't been able to create an isolated setup for reproduction. It appears to be affected by the target server, as practically identical implementations yield different results depending on the call target.

I've been able to trace the leak to this event listener:
https://github.com/bufbuild/connect-es/blob/5930ac12db997ed30620880994a4483f45571db7/packages/connect-node/src/node-universal-client.ts#L292
Which should be cleaned up subsequently in the finally handler on the sentinel:
https://github.com/bufbuild/connect-es/blob/5930ac12db997ed30620880994a4483f45571db7/packages/connect-node/src/node-universal-client.ts#L311-L313
However, stream.session will be undefined after the Http2Stream is destroyed according to the docs, which appears to be happening before the sentinel is finalized when calling certain servers.

The exception from calling off on undefined is then immediately swallowed:
https://github.com/bufbuild/connect-es/blob/5930ac12db997ed30620880994a4483f45571db7/packages/connect-node/src/node-universal-client.ts#L314-L317

This leads to every request assigning a new error event listener on the session, which never gets cleaned up, and node is quick to output a warning after only 10 requests:

I've been able to money-patch this by immediately assigning stream.session to a variable, and disabling the event listener on that inside the sentinel.finally handler instead of accessing the session from the stream object, but I'm not sure if this is the correct fix or if there's an underlying issue.

Environment (please complete the following information):

• @bufbuild/connect-web version: -
• @bufbuild/connect-node version: 0.11.0
• Frontend framework and version: -
• Node.js version: 18.17.0
• Browser and version: -

[timostamm]

Great catch, thank you Simon.

I can reproduce with v0.11.0 and on main:

$ node -v
v18.17.0
$ node --trace-warnings repro-725.mjs
req 0
req 1
req 2
req 3
req 4
req 5
req 6
req 7
req 8
req 9
(node:57858) MaxListenersExceededWarning: Possible EventEmitter memory leak detected. 11 error listeners added to [ClientHttp2Session]. Use emitter.setMaxListeners() to increase limit
    at _addListener (node:events:465:17)
    at ClientHttp2Session.addListener (node:events:487:10)
    at file:///XXX/node_modules/@bufbuild/connect-node/dist/esm/node-universal-client.js:161:24
    at processTicksAndRejections (node:internal/process/task_queues:96:5)

repro-725.mjs

import { createNodeHttpClient, Http2SessionManager } from "@bufbuild/connect-node";
import * as http2 from "http2";
import assert from 'assert'

// start a server that closes all streams with an h2 internal error
const PORT = 8088;
let serverSessions = 0;
const server = http2.createServer()
  .unref()
  .on("session", (sess) => {
    sess.unref();
    serverSessions++;
    assert(serverSessions === 1);
  })
  .on("request", (req, res) => {
    res.writeHead(200);
    setTimeout(() => {
      res.stream.close(http2.constants.NGHTTP2_INTERNAL_ERROR);
    }, 100);
  });
await new Promise(resolve => {
  server.listen(PORT, () => resolve());
})

// create an h2 client
const sm = new Http2SessionManager(`http://localhost:${PORT}`);
const client = createNodeHttpClient({
  httpVersion: "2",
  sessionProvider: () => sm
});

// make requests
for (let i = 0 ; i < 12; i++) {
  console.log("req", i)
  const res = await client({
    url: `http://localhost:${PORT}`,
    method: "POST",
    header: new Headers(),
  });
  try {
    // because the stream is closed after response headers are received,
    // consume the response body to see the closing error code
    for await (const chunk of res.body) {}
  } catch (e) {
    assert(String(e) === "ConnectError: [internal] Stream closed with error code NGHTTP2_INTERNAL_ERROR");
  }
}

[timostamm]

The fix for this was released in v0.12.0.