fix: set network timeout duration for whole the call operation
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.
| 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
| Platform | Engine | Arch | Size (bytes) | Diff |
|---|---|---|---|---|
| ios | - | universal | n/a | -- |
Base commit: 91d16bbd9f8066428209d7b4e602e9991b1a13ec
This is a duplicate of #28491 . Is this going to get merged? Would highly appreciate some attention for this issue
Any updates?
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
Can confirm that we're also affect by the same issue and that this PR fixes it.
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
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
- install
patch-packageas your dev dep - add
patch-packageto yourpostinstallscript in package.json
"postinstall": "patch-package"
- Install fresh dependencies (
yarn install --force) - Locate the file in your
node_modules/react-native/ReactAndroid/src/main/java/com/facebook/react/modules/network/NetworkingModule.java - Find the line which needs to be changed (look at this PR's diff)
- In your root, run
yarn patch-package react-native - This should create a
patches/directory with yourreact-native+version.patch(this file should be small, like this PR's diff) - 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.
If anyone need those changes now in their codebase, you can use patch-package to apply this fix.
How it works
patch-packagecreates 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 innode_modulesyou did are gone, so patch needs to be applied).How to use it
- install
patch-packageas your dev dep- add
patch-packageto yourpostinstallscript in package.json"postinstall": "patch-package"
- Install fresh dependencies (
yarn install --force)- Locate the file in your
node_modules/react-native/ReactAndroid/src/main/java/com/facebook/react/modules/network/NetworkingModule.java- Find the line which needs to be changed (look at this PR's diff)
- In your root, run
yarn patch-package react-native- This should create a
patches/directory with yourreact-native+version.patch(this file should be small, like this PR's diff)- 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.
@Krizzu tried the suggested but apparently this is a
.javafile within thereact-nativelibrary 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.javaafterpatch-packagethe 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.jsonis version0.63.3and afterpatch-packageproduces areact-native-0.63.4but 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-nativeproject 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
@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
Two years later, it's still not resolved.
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.
Does it still valid?
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
This issue has been fixed here https://github.com/facebook/react-native/pull/38953. @krizzu could you close this PR?
Well done @troZee 🎉