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

Timeout does not work at all #440

Closed
tobob opened this issue Apr 3, 2018 · 10 comments
Closed

Timeout does not work at all #440

tobob opened this issue Apr 3, 2018 · 10 comments

Comments

@tobob
Copy link

tobob commented Apr 3, 2018

import fetch from 'node-fetch';

      fetch('https://www.google.com', {
        method: 'GET',
        options: { timeout: 1 },
        timeout: 1,
      })
        .then(res => console.log(res), err => console.log('err2', err))

Am I doing something wrong? That low timeout should return err2 always, or not?
it is always success

using ReactNative, but this should not be an issue
axios however works fine with timeout options

@jeandat
Copy link

jeandat commented Apr 11, 2018

Same here. Timeout is not triggered.

@jeandat
Copy link

jeandat commented Apr 11, 2018

Maybe I missed something nevertheless.

It is written :

// The following properties are node-fetch extensions
follow: 20, // maximum redirect count. 0 to not follow redirect
timeout: 0, // req/res timeout in ms, it resets on redirect. 0 to disable (OS limit applies)
compress: true, // support gzip/deflate content encoding. false to disable
size: 0, // maximum response body size in bytes. 0 to disable
agent: null // http(s).Agent instance, allows custom proxy, certificate etc.

What is concretely a "node-fetch extension"? Is it an external npm package that need to be installed?

@jimmywarting
Copy link
Collaborator

Maybe it shouldn't work... maybe we should remove it altogether? it's not implemented in the spec. This is raising some discussion over at matthew-andrews/isomorphic-fetch#48

The way to handle it in both node and web is thought the upcoming abortable controller.

const controller = new AbortController()
const signal = controller.signal
setTimeout(() => { 
  controller.abort()
}, 1000)

fetch(url, { signal })

@bitinn
Copy link
Collaborator

bitinn commented Apr 11, 2018

timeout is commonly used when you write a web crawler or use public api, where you might want to terminate some long running requests. There were no abort mechanism in Fetch back then, and timeout was the common solution for other http library.

We got tests for timeout if you need some code reference:

https://github.com/bitinn/node-fetch/blob/master/test/test.js#L727-L750

But you can't set it to 1ms and expect it to work, node-fetch use nodejs' built-in http module, and http module need to assign sockets to a request first, for our timeout to work.

https://nodejs.org/api/http.html#http_event_socket
https://github.com/bitinn/node-fetch/blob/master/src/index.js#L55-L62
https://stackoverflow.com/questions/6214902/how-to-set-a-timeout-on-a-http-request-in-node

That said, OP was using React Native. I have no idea how React Native handle this internally, so you better ask on SO or their forum.

@gajus
Copy link
Contributor

gajus commented May 16, 2018

We got tests for timeout if you need some code reference:

I am constantly detecting my scripts hanging on a connection for days even when the timeout is explicitly configured.

Mind you, this happens when connecting through proxy. If that changes anything.

@bitinn
Copy link
Collaborator

bitinn commented May 16, 2018

@gajus I recall someone mentioned that nodejs http module sometimes doesn't bubble the right events when the request got stuck. No concrete report so far. If you see any other network library doing extra work in this space, let us know.

It's kinda strange a stalled connection can last that long though (I would think OS will recycle the socket eventually), are you using any weird settings with http agent or on the OS? are you using continuous object stream? Those are the less tested use cases.

@gajus
Copy link
Contributor

gajus commented May 16, 2018

Nothing unusual in the setup. Basic request using promises. When successful – returns response under a second.

I am going to test/ look into code of https://github.com/sindresorhus/got – it has different level timeouts (connect, socket, and request). Will see if I can replicate the same issue.

@bitinn
Copy link
Collaborator

bitinn commented May 16, 2018

@gajus that might be a good idea, I got the feeling they basically look at the Fetch Spec and replace the parts they don't like. (I always recommend it when people want something that's outside Fetch Spec's scope).

@bitinn
Copy link
Collaborator

bitinn commented Nov 13, 2018

For people in this thread, if timeout is somehow not what you want, node-fetch v2.3.0 now supports AbortSignal which is standard compliant, and should close the stream properly.

(See README for details and examples)

@dsimushkin
Copy link

dsimushkin commented Dec 19, 2022

In case anyone needs to preserve an error stack:

import nodeFetch, { RequestInfo, RequestInit, Response } from "node-fetch";

export function sleep(ms = 0, signal?: AbortSignal): Promise<void> {
  return new Promise<void>((resolve, reject) => {
    const id = setTimeout(() => resolve(), ms);
    signal?.addEventListener("abort", () => {
      clearTimeout(id);
      reject();
    });
  });
}

export async function fetch(
  resource: RequestInfo,
  options?: RequestInit & { timeout: number }
): Promise<Response> {
  const { timeout, signal, ...ropts } = options ?? {};

  const controller = new AbortController();
  let sleepController;
  try {
    signal?.addEventListener("abort", () => controller.abort());

    const request = nodeFetch(resource, {
      ...ropts,
      signal: controller.signal,
    });

    if (timeout != null) {
      sleepController = new AbortController();
      const aborter = sleep(timeout, sleepController.signal);
      const race = await Promise.race([aborter, request]);
      if (race == null) controller.abort();
    }

    return request;
  } finally {
    sleepController?.abort();
  }
}

A full description with reasoning can be found in my stackoverflow answer

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants