grantila / fetch-h2

HTTP/1+2 Fetch API client for Node.js
MIT License
336 stars 16 forks source link

Http1 disconnectAll() doesn't work #101

Closed stefan-guggisberg closed 2 years ago

stefan-guggisberg commented 4 years ago

When making an Http1 request followed by disconnectAll() the process doesn't exit.

When running the following snippet the process hangs, i.e. node doesn't exit. It seems like disconnectAll() doesn't unref the socket.

const { fetch, disconnectAll } = require('fetch-h2');

(async () => {
  const resp = await fetch('http://httpbin.org/status/200');
  await disconnectAll();
}());

Note that this problem is specific to HTTP1(.1) connections, i.e. with HTTP2 the problem doesn't occur. I am therefore intentionally using the http: protocol in order to force an HTTP/1.1 connection.

If you use https: in above example an HTTP2 connection will be negotiated/established and the problem doesn't occur.

While you could argue that this is an edge case because you would normally consume the response body, this is true for 200 responses but not necessarily for 404s etc.

stefan-guggisberg commented 4 years ago

I had reported this issue already in #77 which had been closed. The problem however still exists and can be easily reproduced with the provided example code.

grantila commented 2 years ago

This is a tricky issue, because you've only awaited half of the response. The other half is the payload (whether there is one or not, fetch-h2 can't really know, well, it sometimes could).

If you make sure to always await the response and after that, always await the text() on that response (or body() or json()), then fetch-h2 will know that it can drop the connection. Otherwise, you might want to read the response later at some point.

We don't have good destructor support in JavaScript (yet), so knowing if you still keep a reference to the response, to read the payload later, is impossible.

In browsers, for which fetch was invented, this is not a big deal. Dangling non-dereffed sockets will be closed when you navigate or close the tab.

I'm closing this, but re-open if you think there's a way to mitigate. E.g. fetch-h2 could auto-deref the socket for responses that have no payload (content-length = 0), but then the behaviour would be depending on the response data which isn't ideal neither.

I think it was a mistake of me to link to that issue as "fixed" by a commit, it wasn't.