fix(ext/node): session close during stream setup

[caleblloyd]

• Fixes HTTP2 request opened at same time Close is called throws #25168
• Adds tests for fix(ext/node): http2session ready state #25143

[CLAassistant]

All committers have signed the CLA.

[kt3k]

LGTM

@satyarohith Can you take another look at this?

[caleblloyd]

@satyarohith need me to make any other changes to this PR?

[caleblloyd]

The fix for stream closing was required to correctly implement the test, so that is why I put the fix and the test in the same PR as opposed to putting them in separate PRs

[satyarohith]

My concern with this patch is that it doesn't match node's behaviour exactly but improves it by propagating the error via "error" event instead of throwing it.

Logs:

➜ deno run -A test.mjs 
Script started
Connect callback triggered
'connect' event triggered
Connection established
Request created
Closing client
error: Uncaught (in promise) BadResource: Bad resource ID
    at clientHttp2Request (node:http2:638:16)
    at Object.runMicrotasks (ext:core/01_core.js:690:26)
    at processTicksAndRejections (ext:deno_node/_next_tick.ts:53:10)
    at runNextTicks (ext:deno_node/_next_tick.ts:71:3)
    at eventLoopTick (ext:core/01_core.js:181:21)
    at async node:http2:670:37

➜ denod run -A test.mjs # this PR
Script started
Connect callback triggered
'connect' event triggered
Connection established
Request created
Closing client
'close' event triggered on client
'error' event triggered on request: The pending stream has been canceled
'close' event triggered on request
Script finished

➜ node test.mjs 
Script started
Connect callback triggered
'connect' event triggered
Connection established
Request created
Closing client
'response' event triggered on request
'data' event triggered on request
'end' event triggered on request
'close' event triggered on request
Script finished
'close' event triggered on client

script

import http2 from "node:http2";

const url = "http://127.0.0.1:4246";
const connectPromise = Promise.withResolvers();

console.log("Script started");

const client = http2.connect(url, {}, () => {
  console.log("Connect callback triggered");
  connectPromise.resolve();
});

client.on('connect', () => console.log("'connect' event triggered"));
client.on('error', (err) => console.log("'error' event triggered on client:", err.message));
client.on('close', () => console.log("'close' event triggered on client"));

await connectPromise.promise;
console.log("Connection established");

// leave a request open to prevent immediate destroy
const req = client.request();
console.log("Request created");

req.on("response", (headers) => console.log("'response' event triggered on request"));
req.on("data", (chunk) => console.log("'data' event triggered on request"));
req.on("end", () => console.log("'end' event triggered on request"));
req.on("error", (err) => console.log("'error' event triggered on request:", err.message));
req.on("close", () => {
  console.log("'close' event triggered on request");
  reqClosePromise.resolve();
});

const reqClosePromise = Promise.withResolvers();

console.log("Closing client");
client.close();

await reqClosePromise.promise;
console.log("Script finished");

LGTM - we should match node's behaviour in future iterations.

[satyarohith]

Thanks for the contribution @caleblloyd!

[caleblloyd]

Thanks for the review! I agree, once session.close() implements graceful connection closing the pending request should attempt to be completed, and there should be no error on the request with the test case you posted.