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] How to handle http request timeout #2556

Closed
nosensezzz opened this issue Sep 4, 2015 · 21 comments
Closed

[fetch] How to handle http request timeout #2556

nosensezzz opened this issue Sep 4, 2015 · 21 comments
Labels
Resolution: Locked This issue was locked by the bot.

Comments

@nosensezzz
Copy link

Hi, Guys

I currently ran into a issue that when using fetch to call remote data, I can't handle the timeout situation, if my not return successfully, it will wait forever.

I have google for awhile and I didn't find any good practice to handle, npm fetch doc says it has a timeout keyword in options object, but seems it didn't work at all.

how do you guys handle this situation?

@brentvatne brentvatne changed the title How to handle timeout on react native - fetch(); [fetch] How to handle http request timeout on react native Sep 6, 2015
@brentvatne brentvatne changed the title [fetch] How to handle http request timeout on react native [fetch] How to handle http request timeout Sep 6, 2015
@brentvatne
Copy link
Collaborator

Check out this discussion here: JakeChampion/fetch#175 - there is a solution implemented there that uses promises and seems nice.

Lots of discussion going on about the fetch specification here:
whatwg/fetch#27
whatwg/fetch#20

Another attempt at wrapping fetch to add this: whatwg/fetch#20 (comment)

@narychen
Copy link
Contributor

I really don't agree the above solutions. If fetch wait forever and you use setTimeout to reject the wrapper promise, then the fetch will hold the reference of the resolve and reject forever.
It may seem not so huge of a waster but if the code try to use fetch hundreds of times and timeout happens a lot, then the memory leak will be a problem.

@kyle-ilantzis
Copy link

kyle-ilantzis commented Dec 29, 2016

I agree with @narychen, its a waste of memory. A big discussion is going on over here whatwg/fetch#20 and seems not have any solution. Here's what I did:

  • I copied from node_modules/whatwg-fetch the fetch.js file
  • I modified it to use a setTimeout and then call xhr.abort() (and another modification like fetch.hack = true)
  • I then include my modified whatwg-fetch.js in my index.ios.js and index.android.js

I now have a timeout of 10 seconds on my fetch requests.

You can find my modified whatwg fetch.js file here: https://gist.github.com/kyle-ilantzis/e4521e6194f6c019b14ac91291feb348

You can find the setTimeout I added here: https://gist.github.com/kyle-ilantzis/e4521e6194f6c019b14ac91291feb348#file-whatwg-fetch-js-L440

You can modify 10 seconds to what ever you want here: https://gist.github.com/kyle-ilantzis/e4521e6194f6c019b14ac91291feb348#file-whatwg-fetch-js-L8

I any one is interested, here is the self.fetch.hack = true:
https://gist.github.com/kyle-ilantzis/e4521e6194f6c019b14ac91291feb348#file-whatwg-fetch-js-L4
https://gist.github.com/kyle-ilantzis/e4521e6194f6c019b14ac91291feb348#file-whatwg-fetch-js-L466

@adambene
Copy link

Try https://github.com/benestudio/fetch/tree/feature/timeout

  • npm install whatwg-fetch-timeout --save;
return fetch('/path', {timeout: 500}).then(function() {
  // successful fetch
}).catch(function(error) {
  // network request failed / timeout
})

This fork supports timout just like node-fetch.

This way you can have timeouts runtime and bundle time also. Say goodbye to memory leaks.

Hope it could be merged some sunny day.

@adambene
Copy link

adambene commented Feb 28, 2017

Any idea on how to use my fetch fork without forking and patching RN itself?

@kyle-ilantzis
Copy link

@adambene you want your polyfill to overwrite react-native's polyfill.

The first lines of fetch.js look like

if (self.fetch) {
return
}

Which will be true because the fetch function already exists.

I suggest doing

global.fetch = null;
require('whatwg-fetch-timeout')

(I am not sure but i think it would be in global)

Or changing the if to check for your fetch function such as if (fetch.hasTimeout) { return }

@adambene
Copy link

@kyle-ilantzis
Copy link

@adambene yes whatwg-fetch is already imported. You want your whatwg-fetch-timeout to take precedence. I imagine you are importing yours? The problem is in your fetch.js file at the top of the file there is

if (self.fetch) {
return
}

@adambene
Copy link

adambene commented Feb 28, 2017

@kyle-ilantzis I feel that whatwg-fetch-timeout should only be an extension to whatwg-fetch, so removing the fetch implementation check seems hacky to me.

I've tried npm-shrinkwrap, but does not allow different package names only versions to be overridden.

I am searching for a really maintainable and universal solution, because all my future projects are going to rely on fetch timeout to achieve robustness. That's why forking RN nor hacking whatwg-fetch is not a good option.

(Edit: of course the most welcome would be a warm merge for the timeout feature.)

@adambene
Copy link

adambene commented Feb 28, 2017

It seems that there are 3 choices to override fetch in RN. And an extra.

1 Fork RN

Fork RN and rename the whatwg-fetch dependency to whatwg-fetch-timeout.

Well, it's a maintenance hell.

2 Import whatwg-fetch-timeout in all files

Import whatwg-fetch-timeout in all relevant .js files overriding fetch provided by RN.

Explicit and clean, just don't forget to import everywhere.

3 Override global.fetch

Override global.fetch the hacky way: see also https://github.com/facebook/react-native/blob/dba133a29194e300e9a2e9e6753f9d4e3a13c194/Libraries/Core/InitializeCore.js

4 Don't care

Let pending requests in the air. Pretend if the fetch was timed out without aborting it (https://github.com/building5/promise-timeout) and give you users the option to retry. Watch out for patch and post especially when mobile network is bad. Chaos may happen.

@clshortfuse
Copy link

clshortfuse commented Apr 18, 2017

Use whatwg-fetch-timeout as an alternative

npm install whatwg-fetch-timeout --save

Create a new timeoutFetch function by clearing global.fetch before loading the module.

var timeoutFetch = null;
(()=>{
  var backup = global.fetch;
  global.fetch = null;
  require('whatwg-fetch-timeout');
  timeoutFetch = global.fetch;
  global.fetch = backup;
})();

Then replace your calls for fetch(...) with timeoutFetch(...). Alternatively, you can not restore global.fetch, but that could have some unintended consequences.

@atticoos
Copy link
Contributor

atticoos commented Apr 18, 2017

Our team is currently using robinpowered/react-native-fetch-polyfill.

It essentially provides timeout to the XMLHttpRequest instance, which is already equipped to natively support request timeouts. It's just a problem where fetch does not expose this. The polyfill opens this up.

It currently avoids writing to the global scope, but I suppose we could make that an option

Sample Usage

import fetch from 'react-native-fetch-polyfill';

fetch(path, {timeout: 30 * 1000})

@vijayst
Copy link

vijayst commented May 18, 2017

@adambene For me, importing 'whatwg-fetch-timeout' in all files did not work. I tried the idea by @clshortfuse. The following code did not work:

const backup = global.fetch;
global.fetch = null;
require('whatwg-fetch-timeout');
const timeoutFetch = global.fetch;
global.fetch = backup;

I used timeoutFetch('login', {timeout: 10000, method: 'PUT'});. The call timed out in 90 seconds.

@vijayst
Copy link

vijayst commented May 18, 2017

@adambene Also, why should whatwg-fetch-timeout be imported in all files? Since, we are patching, should it not be done in the main file, say index.android.js?

@vijayst
Copy link

vijayst commented May 18, 2017

@ajwhite I published react-native-fetch-timeout. It seems to set the timeout on xhr. But ontimeout callback is not triggered after the timeout has elapsed.

@adambene
Copy link

adambene commented May 19, 2017

@vijayst because RN hacks fetch into the global. See:

defineProperty(global, 'fetch', () => require('fetch').fetch);

So it is not so straightforward to override that properly.

@adambene
Copy link

For reference I have this question on StackOverflow: http://stackoverflow.com/questions/42495366/react-native-custom-fetch-polyfill

@atticoos
Copy link
Contributor

atticoos commented May 19, 2017

@adambene you should have luck with react-native-fetch-polyfill with the following:

import fetch from 'react-native-fetch-polyfill'

fetch(url, {timeout: 30 * 1000})
  .then(response => {
    // a successful response
  })
  .catch(error => {
    // an error when the request fails, such as during a timeout
  })

This doesn't replace the global.fetch assignment, but it does let you import the polyfilled version with the timeout; which you could give the import a different name to avoid conflicting with the global.fetch name.

@vijayst I have not attempted to replace global.fetch with the polyfill, so I can't speak to that. If global.fetch is defined with writable: false in the Object.defineProperty call, then it will not be replaceable.

@yuanotes
Copy link

yuanotes commented Oct 26, 2017

I am using Promise.race to work around this. Though the fetch promise won't be aborted.

const delay = (timeout, error) => new Promise((resolve, reject) => setTimeout(() => reject(error), timeout))
const result =  await Promise.race([fetchPromise, delay(30 * 1000, 'Timeout Error')]

@adambene
Copy link

@1c7
Copy link

1c7 commented Jan 23, 2018

@ajwhite solution work for me! (React Native 0.52 on Android)

@facebook facebook locked as resolved and limited conversation to collaborators Jul 21, 2018
@react-native-bot react-native-bot added the Resolution: Locked This issue was locked by the bot. label Jul 21, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Resolution: Locked This issue was locked by the bot.
Projects
None yet
Development

No branches or pull requests