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

[Q] timeout handling #175

Closed
gaydenko opened this issue Jul 14, 2015 · 16 comments
Closed

[Q] timeout handling #175

gaydenko opened this issue Jul 14, 2015 · 16 comments

Comments

@gaydenko
Copy link

Hi!

Haven't found the way to set connection timeout. How to?

@dgraham
Copy link
Contributor

dgraham commented Jul 14, 2015

There is an open discussion on timeout handling in the Fetch specification repository: whatwg/fetch#20. We will add the behavior to the polyfill when the feature is added to the standard specification.

@mislav
Copy link
Contributor

mislav commented Jul 28, 2015

You can implement your own timeout wrapper for Promises:

// Rough implementation. Untested.
function timeout(ms, promise) {
  return new Promise(function(resolve, reject) {
    setTimeout(function() {
      reject(new Error("timeout"))
    }, ms)
    promise.then(resolve, reject)
  })
}

timeout(1000, fetch('/hello')).then(function(response) {
  // process response
}).catch(function(error) {
  // might be a timeout error
})

The timeout() function will wrap any promise and ensure that it's at least rejected within ms milliseconds. If the fetch succeeds to resolve earlier than that, then the request will be successful.

Note that this is not a connection timeout. This is a response timeout. Due to technical restrictions we can't implement a connection timeout. Also note that with the above implementation, even if the timeout happens, the original request won't be aborted because we don't have an API to abort fetch requests. Instead, the request will complete in the background but its response will get discarded.

@deju
Copy link

deju commented Mar 9, 2016

@mislav Awesome!

@ghost
Copy link

ghost commented Apr 18, 2016

@mislav Good one.

@nodkz
Copy link

nodkz commented May 4, 2016

Five cents to @mislav solution - remove timeout if promise was resolved or rejected internally.

function timeoutPromise(ms, promise) {
  return new Promise((resolve, reject) => {
    const timeoutId = setTimeout(() => {
      reject(new Error("promise timeout"))
    }, ms);
    promise.then(
      (res) => {
        clearTimeout(timeoutId);
        resolve(res);
      },
      (err) => {
        clearTimeout(timeoutId);
        reject(err);
      }
    );
  })
}

@L-Jovi
Copy link

L-Jovi commented Jun 7, 2016

@nodkz @mislav thanks for your idea!

and how could I use it in ES7 async/await syntax, is't right to code like below?

async request() {
  try { 
    let res = await timeout(1000, fetch('/hello'));
  } catch(error) {
    // might be a timeout error
  }
}

thanks :)

@L-Jovi
Copy link

L-Jovi commented Jun 8, 2016

I ensure above statement can't suitable for ES7 async/await syntax,
but I don't know how to call timeout method, any idea?


update

my test is wrong, above code is right for ES7 async/await fetch, please have a look here and notice @Bergi's reply.

@gre
Copy link

gre commented Jun 21, 2016

i wish this was actually possible to stop the underlying fetch() work...
shortcuting the flow but not interrupting is not really satisfying.
I really wish fetch() was interruptable like in xhr.abort()..

@mislav
Copy link
Contributor

mislav commented Jun 21, 2016

@gre That would be nice, but in reality it's not so trivial to add. See whatwg/fetch#27

@dieseldjango
Copy link

Another 2c to @nodkz enhancement. Don't re-resolve/re-reject the promise if the request does eventually complete after the timeout fires:

function timeoutPromise(ms, promise) {
  return new Promise((resolve, reject) => {
    let timeoutId = setTimeout(() => {
      timeoutId = undefined;
      reject(new Error("promise timeout"))
    }, ms);
    promise.then(
      (res) => {
        if (timeoutId) {
          clearTimeout(timeoutId);
          resolve(res);
        }
      },
      (err) => {
        if (timeoutId) {
          clearTimeout(timeoutId);
          reject(err);
        }
      }
    );
  })
}

@mislav
Copy link
Contributor

mislav commented Mar 7, 2017

@dieseldjango @nodkz I appreciate you trying to improve upon my solution, but you assure you, both approaches are not necessary. Promises can't be re-resolved or re-rejected: the first resolution or rejection "wins" and the others are ignored. Thus, my original code works just as well without any extra code thrown at it:

function timeout(ms, promise) {
  return new Promise(function(resolve, reject) {
    setTimeout(function() {
      reject(new Error("timeout"))
    }, ms)
    promise.then(resolve, reject)
  })
}

If you add extra code that clears the timeout if it won't be necessary, it's a premature optimization. You're basically saving yourself from no more than just an overhead of executing a function that constructs a new Error instance. And, since this function executes async, it's not an overhead at all.

@kuoruan
Copy link

kuoruan commented Feb 2, 2018

What about this?

function timeout (promise, ms = TIME_OUT) {
  let timerPromise = new Promise((resolve, reject) => {
    setTimeout(() => {
      reject(new Error('Request Timeout'))
    }, ms)
  })
  return Promise.race([timerPromise, promise])
}

@FranklinYu
Copy link

#547 may help this.

@PhilSchi
Copy link

PhilSchi commented Mar 23, 2018

I tried a TypeScript Version of @mislav code.

async function timeout<T>(ms, promise): Promise<T> {
    return new Promise<T>(async (resolve, reject) => {
        setTimeout(() => {
            reject(new Error("timeout"))
        }, ms)
        resolve(await promise)
    })
}

@JSONRice
Copy link

JSONRice commented Apr 10, 2018

@mislav it would be great to have a connection timeout. This seems to be a fairly common and needed option going as far back to AngularJS with $http timeout in ms, but as you suggested there are alternatives. Here's an ES6/ES7 compatible approach with a demo (note you will need to Babel this for IE):

/************************************************
 * Asynchronous functions with async and await.
 ***********************************************/
"use strict";
const fetch = require('node-fetch');
// Lots of data (130Mb):
let TARGET_URL = "https://raw.githubusercontent.com/zemirco/sf-city-lots-json/master/citylots.json";

class AjaxService {
    constructor() {
        this.url = TARGET_URL;
        this._timeout = {active: false, ms: 0};
    }

    /************************************************************************************
     * Provide a fetch based promise and set a initiation timer. The Fetch API currently
     * has not mechanism to handle timed out AJAX requests. Here's an explanation from
     * one of the Fetch API maintainers (mislav) on Github:
     *
     * Note that this is not a connection timeout. This is a response timeout. Due to
     * technical restrictions we can't implement a connection timeout. Also note that with
     * the above implementation, even if the timeout happens, the original request won't
     * be aborted because we don't have an API to abort fetch requests. Instead, the
     * request will complete in the background but its response will get discarded.
     *
     * What this function provides is an initiation timeout. If the Fetch request takes
     * more than a set amount of time in ms then reject that request's response data.
     *
     * See: https://github.com/github/fetch/issues/175
     *
     * @param fetchPromise
     * @returns {Promise}
     */
    fetchWithTimeout(fetchPromise) {
        if (this._timeout.active && this._timeout.ms > 0) {
            return new Promise((resolve, reject) => {
                setTimeout(() => {
                    reject(new Error("Fetch is taking to long. Rejecting the response."));
                }, this._timeout.ms);
                fetchPromise.then(resolve, reject);
            });
        } else {
            console.warn("_timeout member needs to be active with seconds greater than zero");
        }
    }

    resetUrl() {
        this.url = TARGET_URL;
    }

    set timeout(timeout) {
	    this._timeout = JSON.parse(JSON.stringify(timeout)); // deep copy
    }

    async asyncFetchData() {
        let response = await ((this._timeout.active && this._timeout.ms > 0) ?
            this.fetchWithTimeout(fetch(this.url)) : (fetch(this.url)));
        if (response.ok) {
    	    return await response.json();
        } else {
            throw new Error(response.status);
        }
    };

    // Run the data fetch function.
    run() {
        this.asyncFetchData()
   	        .then(data => console.log("Data type: ", data.type))
		    .catch(reason => console.error("Caught error: ", reason.message));
    };
}

let ajaxService = new AjaxService();

// Demo the service:
ajaxService.run();

// Now test with an invalid url to see what happens:
ajaxService.url = "bogus";

// This should run first since it's a bogus url:
ajaxService.run();

ajaxService.resetUrl();

// Test the timeout of the initiating request by making a very fast (10 ms) request:
ajaxService.timeout = {active: true, ms: 10};
ajaxService.run();

Tested this out with NodeJS v9.4.0 Works really well and in an asynchronous manner as expected.

@mislav
Copy link
Contributor

mislav commented Apr 11, 2018

it would be great to have a connection timeout

@jasonwr Sure, but we are just a polyfill, and the fetch spec itself doesn't define any timeout functionality. If you want to discuss said features, you can ask in the official repo: whatwg/fetch#20 whatwg/fetch#180

Repository owner locked as resolved and limited conversation to collaborators Apr 11, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests