Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fetch may try to use a closed connection #3492

Open
robhogan opened this issue Aug 21, 2024 · 6 comments
Open

fetch may try to use a closed connection #3492

robhogan opened this issue Aug 21, 2024 · 6 comments
Labels
bug Something isn't working fetch

Comments

@robhogan
Copy link

robhogan commented Aug 21, 2024

Version

v22.6.0

Platform

Darwin MacBook-Pro-6.local 23.5.0 Darwin Kernel Version 23.5.0: Wed May  1 20:12:58 PDT 2024; root:xnu-10063.121.3~5/RELEASE_ARM64_T6000 arm64

Subsystem

http

What steps will reproduce the bug?

// fetch-test.js
const {createServer} = require('node:http');

const port = 8080;
const url = 'http://localhost:' + port;

const server = createServer((req, res) => res.end()).listen(port, async () => {
  await fetch(url);
  server.closeIdleConnections();

  setImmediate(async () => {
    await fetch(url); // Throws TypeError with cause UND_ERR_SOCKET or ECONNRESET
    server.close();
  });
});

How often does it reproduce? Is there a required condition?

Reproduces consistently for me but the error cause varies roughly evenly between ECONNRESET and UND_ERR_SOCKET (details below)

What is the expected behavior? Why is that the expected behavior?

fetch creates a new connection if there are none open, and the request succeeds.

What do you see instead?

node fetch-test.js
node:internal/deps/undici/undici:13178
      Error.captureStackTrace(err);
            ^

TypeError: fetch failed
    at node:internal/deps/undici/undici:13178:13
    at async Immediate.<anonymous> (/Users/robhogan/workspace/fetch-test.js:11:5) {
  [cause]: SocketError: other side closed
      at Socket.<anonymous> (node:internal/deps/undici/undici:6020:28)
      at Socket.emit (node:events:532:35)
      at endReadableNT (node:internal/streams/readable:1696:12)
      at process.processTicksAndRejections (node:internal/process/task_queues:82:21) {
    code: 'UND_ERR_SOCKET',
    socket: {
      localAddress: '::1',
      localPort: 57996,
      remoteAddress: undefined,
      remotePort: undefined,
      remoteFamily: undefined,
      timeout: undefined,
      bytesWritten: 338,
      bytesRead: 122
    }
  }
}

Node.js v22.6.0

OR

node fetch-test.js
node:internal/deps/undici/undici:13178
      Error.captureStackTrace(err);
            ^

TypeError: fetch failed
    at node:internal/deps/undici/undici:13178:13
    at async Immediate.<anonymous> (/Users/robhogan/workspace/fetch-test.js:11:5) {
  [cause]: Error: read ECONNRESET
      at TCP.onStreamRead (node:internal/stream_base_commons:218:20) {
    errno: -54,
    code: 'ECONNRESET',
    syscall: 'read'
  }
}

Node.js v22.6.0

Additional information

This seems to be quite sensitive to timing/the event loop in a way I haven't pinned down.

  • The setImmediate (or setTimeout(cb, 0)) is required to reproduce the issue.
  • Adding another setImmediate before the second fetch makes it succeed.
  • Adding {headers:{'Connection': 'close'}} to the first request succeeds.
@RedYetiDev
Copy link
Member

const {createServer} = require('node:http');

const port = 8080;
const url = 'http://localhost:' + port;

const server = createServer((req, res) => res.end()).listen(port, async () => {
  await fetch(url);
  server.closeIdleConnections();

  setImmediate(async () => {
    await fetch(url); // Throws TypeError with cause UND_ERR_SOCKET or ECONNRESET
    server.close();
  });
});
$ node repro.js
node:internal/deps/undici/undici:13178
      Error.captureStackTrace(err);
            ^

TypeError: fetch failed
    at node:internal/deps/undici/undici:13178:13
    at async Immediate.<anonymous> (/repro.js:12:5) {
  [cause]: Error: read ECONNRESET
      at TCP.onStreamRead (node:internal/stream_base_commons:218:20) {
    errno: -54,
    code: 'ECONNRESET',
    syscall: 'read'
  }
}

Node.js v22.6.0

vzaidman referenced this issue in facebook/metro Aug 21, 2024
Summary:
Pull Request resolved: #1327

Updating `node-fetch` to the latest version caused tests in `packages/metro/src/integration_tests/__tests__/server-test.js` to fail. Running each test case inside the file failed individually, but there seems to have been a race condition between the tests causing subsequent tests to fail with the following error:
```
FetchError: request to http://localhost:10028/import-export/index.bundle?platform=ios&dev=true&minify=false failed, reason: socket hang up
```
It happens because the "connection: close" header is removed in the latest version of `node-fetch`: node-fetch/node-fetch#1765 and can be fixed by adding this header manually to fetches.
```
fetch('path', {headers: {'Connection': 'close'}});
```
This diff introduces a workaround where we add that header, however `fetch` is expected to work even without it when the following bug is fixed: https://github.com/nodejs/node/issues/54484

Reviewed By: robhogan

Differential Revision: D61336391
facebook-github-bot referenced this issue in facebook/metro Aug 21, 2024
Summary:
Pull Request resolved: #1327

Updating `node-fetch` to the latest version caused tests in `packages/metro/src/integration_tests/__tests__/server-test.js` to fail. Running each test case inside the file failed individually, but there seems to have been a race condition between the tests causing subsequent tests to fail with the following error:
```
FetchError: request to http://localhost:10028/import-export/index.bundle?platform=ios&dev=true&minify=false failed, reason: socket hang up
```
It happens because the "connection: close" header is removed in the latest version of `node-fetch`: node-fetch/node-fetch#1765 and can be fixed by adding this header manually to fetches.
```
fetch('path', {headers: {'Connection': 'close'}});
```
This diff introduces a workaround where we add that header, however `fetch` is expected to work even without it when the following bug is fixed: https://github.com/nodejs/node/issues/54484

Also, since `node-fetch` was only used in this one test, it was actually a good opportunity to remove it from the project's dependencies altogether.

Reviewed By: robhogan

Differential Revision: D61336391

fbshipit-source-id: 898c9745dbbc1e622699ff36b4112a95c3c68656
@KhafraDev KhafraDev transferred this issue from nodejs/node Aug 22, 2024
@Uzlopak
Copy link
Contributor

Uzlopak commented Aug 22, 2024

If I put following line in setImmediate then it works as expected, but other tests fail

this[kResume](true)

@jazelly
Copy link
Contributor

jazelly commented Aug 22, 2024

I don't think this is an undici specific issue, as same thing would happen by using native http.request in node with keepAlive.

This seems to be an limitation with keepAlive implementation. The second request is assigned to the socket that is about to destroy, as it is not aware of that.

@jakecastelli
Copy link
Contributor

jakecastelli commented Aug 22, 2024

To extend @jazelly's point - here is a small repo with node:http

// fetch-test.js
import { createServer, request } from "node:http";

const port = 8084;
const url = new URL("http://localhost:" + port);

const server = createServer((req, res) => res.end()).listen(port, async () => {
  function httpRequestPromise(reqOptions) {
    return new Promise((resolve, reject) => {
      const req = request(reqOptions, (res) => {
        console.log(`STATUS: ${res.statusCode}`);
        res.setEncoding("utf8");
        res.on("data", () => {});
        res.on("end", () => {
          resolve();
        });
      });

      req.on("error", (error) => {
        console.error(`Request error: ${error.message}`);
        reject(error);
      });
      req.end();
    });
  }

  await httpRequestPromise({
    headers: {
      // 'Content-Type': 'application/json',
      Connection: "keep-alive",
    },
    hostname: url.hostname,
    port: url.port,
    path: url.pathname,
  });

  server.closeIdleConnections();
  console.log("idle connections should have closed...");

  setImmediate(async () => {
    await httpRequestPromise({
      hostname: url.hostname,
      port: url.port,
      path: url.pathname,
    });
    server.close();
  });
});

when run this you should see:

idle connections should have closed...
Request error: read ECONNRESET
node:internal/process/promises:394
    triggerUncaughtException(err, true /* fromPromise */);
    ^

Error: read ECONNRESET
    at TCP.onStreamRead (node:internal/stream_base_commons:218:20) {
  errno: -54,
  code: 'ECONNRESET',
  syscall: 'read'
}

or

idle connections should have closed...
Request error: socket hang up
node:_http_client:530
    emitErrorEvent(req, new ConnResetException('socket hang up'));
                        ^

Error: socket hang up
    at Socket.socketOnEnd (node:_http_client:530:25)
    at Socket.emit (node:events:532:35)
    at endReadableNT (node:internal/streams/readable:1696:12)
    at process.processTicksAndRejections (node:internal/process/task_queues:90:21) {
  code: 'ECONNRESET'
}

if we have NODE_DEBUG=net then we can see:

Details
NET 49784: setupListenHandle null 8084 4 0 undefined
NET 49784: setupListenHandle: create a handle
NET 49784: bind to ::
NET 49784: createConnection [
  [Object: null prototype] {
    headers: { Connection: 'keep-alive' },
    hostname: 'localhost',
    port: '8084',
    path: null,
    host: 'localhost',
    keepAlive: true,
    scheduling: 'lifo',
    timeout: 5000,
    noDelay: true,
    servername: 'localhost',
    _agentKey: 'localhost:8084:',
    encoding: null,
    keepAliveInitialDelay: 1000
  },
  [Function (anonymous)],
  [Symbol(normalizedArgs)]: true
]
NET 49784: pipe false null
NET 49784: connect: find host localhost
NET 49784: connect: dns options { family: undefined, hints: 1024 }
NET 49784: connect: autodetecting
NET 49784: _read - n 16384 isConnecting? true hasHandle? true
NET 49784: _read wait for connection
NET 49784: connect/multiple: only one address found, switching back to single connection
NET 49784: connect: attempting to connect to 127.0.0.1:8084 (addressType: 4)
NET 49784: afterConnect
NET 49784: _read - n 16384 isConnecting? false hasHandle? true
NET 49784: Socket._handle.readStart
NET 49784: onconnection
NET 49784: _read - n 16384 isConnecting? false hasHandle? true
NET 49784: Socket._handle.readStart
STATUS: 200
NET 49784: _read - n 16384 isConnecting? false hasHandle? true
NET 49784: destroy
NET 49784: close
NET 49784: close handle
NET 49784: has server
NET 49784: SERVER _emitCloseIfDrained
NET 49784: SERVER handle? true   connections? 0
idle connections should have closed...
NET 49784: emit close
NET 49784: destroy
NET 49784: close
NET 49784: close handle
Request error: read ECONNRESET
node:internal/process/promises:391
    triggerUncaughtException(err, true /* fromPromise */);
    ^

Error: read ECONNRESET
    at TCP.onStreamRead (node:internal/stream_base_commons:218:20) {
  errno: -54,
  code: 'ECONNRESET',
  syscall: 'read'
}

Close event was emitted after closeIdleConnections

NET 39667: SERVER _emitCloseIfDrained
NET 39667: SERVER handle? true   connections? 0
idle connections should have closed...  <------------- this is the console log after closeIdleConnections
NET 39667: emit close
NET 39667: destroy
NET 39667: close
NET 39667: close handle

@KhafraDev
Copy link
Member

@nodejs/http

@mcollina
Copy link
Member

There are is only one thing to do: always retry the request if the other side is closed. This would mean configuring the retry interceptor by default for fetch also for POST etc in case of a network error.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working fetch
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants