Timeout implementation of fetch

Question

After doing some research on google i didn't find any good info on how to implement timeout with fetch (looks like there are multiple proposals but that's about it so nothing has implemented it). For what i'm doing i absolutely need i though. So i came up with simple solution that i would like a code review on.

 var responseReturned = false;
 var timedOut = false;
 fetch(url,
 {
 method: "post",
 credentials: "same-origin"
 })
 .then((res) => {
 responseReturned = true;
 checkResponseStatus(res);
 })
 .then(function () {
 if (!timedOut) {
 //some screen logic
 }
 })
 .catch(function (ex) {
 if (!timedOut) {
 //handle errors
 }
 });
 setTimeout(() => {
 if (!responseReturned) {
 timedOut = true;
 spanSubmitErr.style.display = "inline";
 }
 }, 8000);

The issue we are having is with clients is the JavaScript is doing a HTTP Post to our servers. We have had some clients that their ISP was having connection issues so their networking equipment was dropping packets. We have had others that they lost connection so javascript never got a response from our servers. So what i'm trying to do is to say if the javascript doesn't receive a response back within 8 seconds that the page is going to assume something went wrong.

• James: Using a flag needs like a reasonable approach to me, only thing I would say is I'm not sure you'd still want to process the response in the case of a timeout? Maybe I'm not understanding the point of the timeout but I gathered the idea is if it times out the request has in-effect failed.
• Icepickle: Is a timeout not basically an error?, your method won't really do anything till the server either timed out or threw an exception or responded. Maybe you have to create your own fetch?
• coding4fun: hi guys and thanks for the responses. What we are dealing with is people who have intermittent connection issues. So they submit (POST) and it hits our servers but the javascript never gets a response so it sits there forever because of a connection issue on their side.
• Icepickle: Can't you then just wrap fetch in your own method and reject / throw an error in case you hit your timeout?
• James: @coding4fun so yeah, think you'd need to throw (or do something) from your timeout in that case as you'll want the client to action the fact the response has timed out. If the request does eventually come back, you would ignore it.

Answer 1

Personally, I think the flag is not the best idea, or at least not, in the scenario you have added it, as it looks like a global variable.

The scenario you have set up now, poses a few problems as you present it, nl:

setTimeout(() => {
 if (!responseReturned) {
 timedOut = true;
 spanSubmitErr.style.display = "inline";
 }
}, 8000);

This doesn't interact with the request at all, at most, the user will see something on his screen, however the response will still be running, and your code may eventually react on it, so you are pretty much unsure if it really worked.

But don't forget that any callback you create should be canceled in the end (through setTimeout (see PatrickRoberts comment below) or setInterval), so that there is no need for you to check if a response returned ;). I personally still prefer the setInterval and clearInterval methods to do that.

Suggested rewrite ES5

function fetch() {
 return new Promise( function (resolve) {
 var timeout = (Math.random() * 10 + 1) * 1000;
 console.log('Waiting ' + timeout + ' ms');
 setTimeout( function() {
 resolve();
 }, timeout);
 });
}
function _fetch( options ) {
 if (!options) {
 options = {
 timeout: 5000
 };
 }
 return new Promise( function(resolve, reject) {
 var interval = setInterval( function() {
 clearInterval( interval );
 reject( { statusCode: 504, status: 'timeout - client' } );
 }, options.timeout || 5000 );
 fetch().then( function( response ) {
 clearInterval( interval );
 resolve( response );
 } ).catch( function( error ) {
 clearInterval( interval );
 reject( error );
 } );
 });
}
function test() {
 var internal = function q() {
 _fetch().then( function() { 
 setTimeout( function() { 
 q(); 
 }, 0 ); 
 }).catch( function( result ) {
 console.log( result );
 });
 };
 internal();
}
test();

Suggested rewrite ES6

If your code base is not to large, or if you are willing to put the time in for refactoring, I would suggest you wrap fetch with your own function, like in the following code sample.

You know that fetch will return a Promise, so create your own version of fetch, which you could then export if necessary, and redirect your files that point to the fetch functionality to your own implementation.

// random method that waits certain time, then resolves
const fetch = () => {
 let timeout = (Math.random() * 10 + 1) * 1000;
 console.log(`waiting ${timeout}`);
 return new Promise((resolve) => {
 setTimeout( () => resolve(), timeout); 
 });
}
const _fetch = ( opt = { timeout: 5000 } ) => {
 return new Promise( (resolve, reject) => {
 console.time('interval');
 let interval = setInterval(() => {
 console.timeEnd('interval');
 clearInterval( interval );
 reject({ message: 'timeout occured', statusCode: 504, status: 'timeout - client' });
 }, opt.timeout );
 fetch().then( (...args) => {
 console.timeEnd('interval');
 clearInterval( interval );
 resolve(...args);
 }).catch( (...args) => {
 console.timeEnd('interval');
 clearInterval( interval );
 reject(...args);
 });
 } );
}
async function runRandomTest() {
 let ok = 1;
 while (ok) {
 await _fetch().catch( (...args) => {
 console.log(...args);
 ok = false;
 });
 }
 console.log('test completed');
}
runRandomTest();

Explanation of rewrite

The biggest change would be here, that the fetch function gets wrapped, and called from _fetch, which you would then use inside your code

If you look at it, it works slightly different, nl:

function _fetch( options ) {
 if (!options) {
 options = {
 timeout: 5000
 };
 }
 return new Promise( function(resolve, reject) {
 var interval = setInterval( function() {
 clearInterval( interval );
 reject( { statusCode: 504, status: 'timeout - client' } );
 }, options.timeout || 5000 );
 fetch().then( function( response ) {
 clearInterval( interval );
 resolve( response );
 } ).catch( function( error ) {
 clearInterval( interval );
 reject( error );
 } );
 });
}

It returns it's own promise, and uses the setInterval. In case the result is returned (or another error is thrown), the clearInterval is called and the promise gets handled by either resolve or reject. In case the interval occurs, the promise gets rejected, and the clearInterval is also called.

Note that if you run the test (at least the es6 one), you can see the time it took before the timeout occured, and it will not run longer. Neither does the then method of the fetch still execute. It is true however that your httprequest will still be open, but I don't think that should be such a huge problem, as it will not interact with your own code anymore.

It is still important though to handle the catch of the fetch and pipe it through, so that the interval gets cleared, but that you also get notified of the other errors that might have occured during the request.

• Patrick Roberts: A better option [than setTimeout] would be the setInterval as this can be cleared -- hold on, are you trying to say that setTimeout() can't be cleared???
• Icepickle: @PatrickRoberts Oh cool :) I never realized :) Thanks
• Knu: Using a Gateway Timeout for a client side timeout is incorrect; a 408 wouldn't be appropriate either.
• Icepickle: @Knu but I am not returning a 408 anywhere only the 504 code, this was just how he could at least get some kind of error code back

Answer 2

Suggested ES6 rewrite

function timeoutFetch (input, init = {}) {
 const timeout = init && Number(init.timeout) || 8000
 return new Promise((resolve, reject) => {
 fetch(input, init).then(resolve, reject)
 setTimeout(() => reject(new TypeError('Client timed out')), timeout)
 })
}

Invocation

timeoutFetch(url, {
 method: 'post',
 credentials: 'same-origin',
 timeout: 8000 // optional
}).then(checkResponseStatus, error => {
 // could be fetch() error or timeout error
})

Explanation

function timeoutFetch (input, init = {})

Here, we are mimicking the signature of the native fetch API, which has one required argument and one optional argument. We simply expect a timeout property to optionally exist on init within this wrapper function.

const timeout = init && Number(init.timeout) || 8000

This first checks that init is "truthy", and then checks that init.timeout is a valid, non-zero number. If these are satisfied, then timeout is assigned the value of the supplied property, otherwise it defaults to 8000 milliseconds, like in your example.

return new Promise((resolve, reject) => { ... })

If you are at all familiar with using promises, then you'll recognize this pattern. While it is typically considered an anti-pattern, this particular implementation is written properly, and is also necessary in this case to take advantage of a convenient implicit race-condition behavior of promises that I'll explain in a moment.

fetch(input, init).then(resolve, reject)

This line invokes the native fetch() method with the wrapper's arguments and resolves the explicitly constructed promise with the fulfilled Response object, if, and only if, it is successful and it completes before the setTimeout() callback is invoked.

The reason this occurs is because of the specification: a promise can only be fulfilled, rejected, or remain pending forever, and if it is fulfilled or rejected, it is "settled" and cannot be fulfilled or rejected again.

If it is unsuccessful, and it fails before the timeout occurs, then it will invoke reject() with an error.

setTimeout(() => ...), timeout)

This part is pretty straightforward; we're creating a timeout given the amount of milliseconds supplied from init.timeout or the default 8000, and invoking the callback function in that amount of time.

reject(new TypeError('Client timed out'))

Inside the callback, we're rejecting the constructed promise with a TypeError, but keep in mind, if the fetch() function has already invoked resolve() or reject() in its then(), this call is essentially a noop because the constructed promise has already locked into its state and cannot be "settled" again. Because of this, it is unnecessary to assign a reference to the timeout and call clearTimeout() if those occur first.

Conclusion

If you've read this far, you've probably realized by now that the suggested code is compact because it takes advantage of a thorough understanding of the promise specification, and is able to safely make assumptions that keeps the code DRY.