[node v20] Fetch with default context and HTTP/1.1 server hangs when not reading the response body

[jdelbick]

Description
For http2, the stream doesn't get closed if the body is not read. Not reading the body is common sometimes for error cases in nodejs and this error doesn't happen if you use HTTP1 or other node fetch implementations like node-fetch. Leaving connections open can result in hanging in tests or extreme cases if the service calls a lot of HTTP requests that fail, it could result in

ERROR	(node:8) MaxListenersExceededWarning: Possible EventEmitter memory leak detected. 1001 error listeners added to [ClientHttp2Session]. Use emitter.setMaxListeners() to increase limit
(Use `node --trace-warnings ...` to show where the warning was created)

My suggestion is to have some warning about this and suggest to always read the body in http2.

To Reproduce
This script displays hanging when running locally with any value of times >1. (node v20)

import { fetch } from '@adobe/fetch';
async function fetchUrl(url) {
    const response = await fetch(url);
    if (!response.ok) {
        throw new Error(`Failed to fetch ${url}: ${response.status} ${response.statusText}`);
    }
    const data = await response.text();
    return data;
}

async function fetchMultipleTimesSync(url, times) {
    for (let i = 1; i <= times; i++) {
        try {
            await fetchUrl(url);
        }
        catch (error) {
            console.error(error);
        }
    }
}

const url = 'https://the-internet.herokuapp.com/status_codes/500';
const times = 2;
await fetchMultipleTimesSync(url, times)

node-fetch doesn't hang in this case:

import fetch from 'node-fetch';
async function fetchUrl(url) {
    const response = await fetch(url);
    if (!response.ok) {
        throw new Error(`Failed to fetch ${url}: ${response.status} ${response.statusText}`);
    }
    const data = await response.text();
    return data;
}
// same code as above ...

However if you just switch the body read to before the error check, it works fine

import { fetch } from '@adobe/fetch';
async function fetchUrl(url) {
    const response = await fetch(url);
    const data = await response.text();
    if (!response.ok) {
        throw new Error(`Failed to fetch ${url}: ${response.status} ${response.statusText}`);
    }
    // const data = await response.text();
    return data;
}

async function fetchMultipleTimesSync(url, times) {
    for (let i = 1; i <= times; i++) {
        try {
            await fetchUrl(url);
        }
        catch (error) {
            console.error(error);
        }
    }
}

// const url = 'https://example.com';
const url = 'https://the-internet.herokuapp.com/status_codes/500';
const times = 2;
await fetchMultipleTimesSync(url, times)

Expected behavior
adobe/fetch docs should call this out, along the lines of “you normally don’t have to do this with other fetch implementations, but in our case cleanup only happens after the response body is consumed. if not, leaks can happen. make sure to always consume the response in all cases (error or not) with await response.body() [or response.text()]”

Version:
v4.1.2

[stefan-guggisberg]

Thanks for reporting this issue and providing detailed instructions to reproduce it!

The observed behavior is due to internal changes in Node's http2 implementation. The changes were apparently introduced in Node v20. The behavior is not reproducible on Node v18.

node-fetch doesn't hang in this case:

node-fetch does not support Http/2.

Workarounds:

1. it's good practice to always consume the response body.
2. use Http/1.1, e.g.

import { h1 } from '@adobe/fetch';
const { fetch } = h1();

3. use Node v18

adobe/fetch should behave the same on different Node versions. I'll investigate what exactly is causing this change of behavior. However don't hold your breath. It's gonna take a while.

[stefan-guggisberg]

Found the root cause. It's unrelated to HTTP/2 but specific to HTTP/1.1. The observed behavior is due to a change in Node.js: As ofv19.0.0 the global agent ((http|https).globalAgent) has keep-alive enabled by default.

adobe/fetch uses the global agent implicitly, e.g. when using the default context and requesting from a server that only supports HTTP/1.1.

Some links:
https://nodejs.org/api/http.html#class-httpagent
nodejs/node#37184
https://github.com/nodejs/node/pull/43522/files#diff-494d2deee304c672124ecd82d090283595fd3d8c5a80a1825d972a2d229e4944L334-R334

[stefan-guggisberg]

The hanging process when not reading the response (due to dangling sockets) is not specific to adobe/fetch. It can be reproduced on Node.js v19+ with this plain vanilla node script:

import https from 'node:https';

const url = new URL('https://the-internet.herokuapp.com/status_codes/500');

let req = https.request(url, (res) => {
  console.log('req #1 statusCode:', res.statusCode);

  // don't read body
  // res.on('data', (d) => {
  //   process.stdout.write(d);
  // });
});
req.end();

req = https.request(url, (res) => {
  console.log('req #2 statusCode:', res.statusCode);

  // don't read body
  // res.on('data', (d) => {
  //   process.stdout.write(d);
  // });
});
req.end();

If the response body is consumed or when using Node.js v18 the process terminates normally.

[alexkli]

I can confirm with curl that https://the-internet.herokuapp.com (used in the test case above) only supports HTTP/1.1.

[stefan-guggisberg]

fixed by #476