react-native icon indicating copy to clipboard operation
react-native copied to clipboard

fix: set network timeout duration for whole the call operation

Open krizzu opened this issue 5 years ago • 14 comments

Summary

Networking module for Android has been incorrectly setting timeout duration - the value has been set only for the duration of the connection to the host, instead of the whole network operation (sending/reading body). Because of that, the long standing API call would timeout, even though the default value of timeout is 0ms (no timeout) (for axios, it's the cryptic "Timeout of 0ms exceeded" error). That happens because OkHttp (used for networking under the hood) has default values of 10s for writing/reading operation, which are not changed.

This PR fixes that by applying the value of timeout for the whole operation.

Thanks @wojteg1337 for help to nail down the root of the issue 💪

Changelog

[Android] [Changed] - Correctly set timeout for network calls

Test Plan

Green CI.

krizzu avatar Jul 22 '20 19:07 krizzu

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 6,764,834 4
android hermes armeabi-v7a 6,432,482 4
android hermes x86 7,152,662 5
android hermes x86_64 7,042,564 0
android jsc arm64-v8a 8,936,087 7
android jsc armeabi-v7a 8,596,122 0
android jsc x86 8,767,038 -3
android jsc x86_64 9,342,572 0

Base commit: 91d16bbd9f8066428209d7b4e602e9991b1a13ec

analysis-bot avatar Jul 22 '20 20:07 analysis-bot

Platform Engine Arch Size (bytes) Diff
ios - universal n/a --

Base commit: 91d16bbd9f8066428209d7b4e602e9991b1a13ec

analysis-bot avatar Jul 22 '20 20:07 analysis-bot

This is a duplicate of #28491 . Is this going to get merged? Would highly appreciate some attention for this issue

jnurkka avatar Oct 30 '20 09:10 jnurkka

Any updates?

wojteg1337 avatar Nov 23 '20 10:11 wojteg1337

Perhaps Im new... but what is the CircleCI test_windows trying to do? Hoping this Branch can be merge in soon, because we are also facing the same issue with setting timeout using axios

Vincenttbk avatar Apr 26 '21 08:04 Vincenttbk

Can confirm that we're also affect by the same issue and that this PR fixes it.

mariomc avatar Apr 27 '21 15:04 mariomc

Or is there anyway tha tI can run from this branch? Because i tried pointing to the branch in my package.json, but it seems like it is not getting changes from class

Vincenttbk avatar Apr 28 '21 03:04 Vincenttbk

If anyone need those changes now in their codebase, you can use patch-package to apply this fix.

Note: This will only work if you build React Native locally, so that you can apply changes to your React Native module itself. Here's the guide how could you go about building from source

How it works

patch-package creates a file with git patch with changes you made to the library in your node modules. Then this patch will be applied whenever you install your dependencies (if you install deps, all changes in node_modules you did are gone, so patch needs to be applied).

How to use it

  1. install patch-package as your dev dep
  2. add patch-package to your postinstall script in package.json
"postinstall": "patch-package"
  1. Install fresh dependencies (yarn install --force)
  2. Locate the file in your node_modules/react-native/ReactAndroid/src/main/java/com/facebook/react/modules/network/NetworkingModule.java
  3. Find the line which needs to be changed (look at this PR's diff)
  4. In your root, run yarn patch-package react-native
  5. This should create a patches/ directory with your react-native+version.patch (this file should be small, like this PR's diff)
  6. And you're done - this patch will be applied every time when you yarn install (CI, locally etc.). Just remember to re-build (re-install) Android app, since those are native changes.

krizzu avatar Apr 28 '21 08:04 krizzu

If anyone need those changes now in their codebase, you can use patch-package to apply this fix.

How it works

patch-package creates a file with git patch with changes you made to the library in your node modules. Then this patch will be applied whenever you install your dependencies (if you install deps, all changes in node_modules you did are gone, so patch needs to be applied).

How to use it

  1. install patch-package as your dev dep
  2. add patch-package to your postinstall script in package.json
"postinstall": "patch-package"
  1. Install fresh dependencies (yarn install --force)
  2. Locate the file in your node_modules/react-native/ReactAndroid/src/main/java/com/facebook/react/modules/network/NetworkingModule.java
  3. Find the line which needs to be changed (look at this PR's diff)
  4. In your root, run yarn patch-package react-native
  5. This should create a patches/ directory with your react-native+version.patch (this file should be small, like this PR's diff)
  6. And you're done - this patch will be applied every time when you yarn install (CI, locally etc.). Just remember to re-build (re-install) Android app, since those are native changes.

@Krizzu tried the suggested but apparently this is a .java file within the react-native library which requires a rebuild of this entire library before the changes can be reflected within the generated jar/arr file.

I realised that if by just looking at the node_modules/react-native/ReactAndroid/src/main/java/com/facebook/react/modules/network/NetworkingModule.java after patch-package the changes were there, but when trying to locate the file within Android Studio for this "patched" aar, it is still remain as before changed.

For instance the react-native version in package.json is version 0.63.3 and after patch-package produces a react-native-0.63.4 but the jar file (react-native-0.63.4-sources.jar) within this patch folder doesn't reflect the changes.

Wondering how did you manage to do that? did you rebuild the react-native project to generate a new aar/jar for whichever projects that requires this change? Thanks in advance.

MystL avatar Apr 28 '21 15:04 MystL

@Krizzu tried the suggested but apparently this is a .java file within the react-native library which requires a rebuild of this entire library before the changes can be reflected within the generated jar/arr file.

I realised that if by just looking at the node_modules/react-native/ReactAndroid/src/main/java/com/facebook/react/modules/network/NetworkingModule.java after patch-package the changes were there, but when trying to locate the file within Android Studio for this "patched" aar, it is still remain as before changed.

For instance the react-native version in package.json is version 0.63.3 and after patch-package produces a react-native-0.63.4 but the jar file (react-native-0.63.4-sources.jar) within this patch folder doesn't reflect the changes.

Wondering how did you manage to do that? did you rebuild the react-native project to generate a new aar/jar for whichever projects that requires this change? Thanks in advance.

Exactly, per default gradle doent build the react-native android native modules but downloads them from a remote repository.

We patched the java code and used these instructions for building the native modules from node_modules. It wasn't super straightforward, but if you run into issues, I can try to help

jnurkka avatar Apr 28 '21 19:04 jnurkka

@MystL My apologies and You're right - Android comes already pre-build, so this patch-package won't work directly. I remember now that what we did to make this work was instead copying over the Networking module implementation, including it as a new native module and patch-packaged the xhrto use it instead of bundled Networking one.

So either building from source + patching the change OR one could also go around and modify the OkHttp client timeout values via builder instead.

For example, in your MainApplication.java, you can do this in getPackages method:

@Override
        protected List<ReactPackage> getPackages() {
          
          // ....    

          NetworkingModule.CustomClientBuilder builder = new NetworkingModule.CustomClientBuilder() {
            @Override
            public void apply(OkHttpClient.Builder builder) {
              // play with values, 0 means no timeout
              builder.callTimeout(30000, TimeUnit.MILLISECONDS);
              builder.readTimeout(0, TimeUnit.MILLISECONDS);
              builder.writeTimeout(0, TimeUnit.MILLISECONDS);
              builder.connectTimeout(0, TimeUnit.MILLISECONDS);
            }
          };

          NetworkingModule.setCustomClientBuilder(builder);
          return packages;
        }

~I don't guarantee this will work perfectly, since you have to keep in sync with timeout in your http client used on JS side (otherwise, you might get Network request failed instead of Timeout error)~

Actually, I see this will be fine, since xhr does not distinguish Timeout error

krizzu avatar Apr 29 '21 07:04 krizzu

Two years later, it's still not resolved.

1280103995 avatar Jul 27 '22 02:07 1280103995

This PR is stale because it has been open 180 days with no activity. Remove stale label or comment or this will be closed in 7 days.

github-actions[bot] avatar Jun 14 '23 05:06 github-actions[bot]

Does it still valid?

troZee avatar Jun 14 '23 06:06 troZee

Someone tried to solve this problem since I have the same problem, when using axios in react native the timeouts in the requests do not work correctly when I am on an unstable network, I perform the cancellation by token or using abortcontroller but when I recover again the connection, future requests are no longer made until I restart the app

Marco29001 avatar Sep 29 '23 21:09 Marco29001

This issue has been fixed here https://github.com/facebook/react-native/pull/38953. @krizzu could you close this PR?

troZee avatar Oct 03 '23 13:10 troZee

Well done @troZee 🎉

krizzu avatar Oct 03 '23 13:10 krizzu