Skip to content

fix: set network timeout duration for whole the call operation#29471

Closed
krizzu wants to merge 1 commit into
react:mainfrom
krizzu:network-timeout
Closed

fix: set network timeout duration for whole the call operation#29471
krizzu wants to merge 1 commit into
react:mainfrom
krizzu:network-timeout

Conversation

@krizzu

@krizzu krizzu commented Jul 22, 2020

Copy link
Copy Markdown

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.

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. p: Callstack Partner: Callstack Partner labels Jul 22, 2020
@react-native-bot react-native-bot added the Platform: Android Android applications. label Jul 22, 2020
@analysis-bot

Copy link
Copy Markdown
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: 91d16bb

@analysis-bot

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

Base commit: 91d16bb

@jnurkka

jnurkka commented Oct 30, 2020

Copy link
Copy Markdown

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

@wojteg1337

Copy link
Copy Markdown
Contributor

Any updates?

@LironSherk LironSherk left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

had the same issue, will solve the bug

@Vincenttbk

Vincenttbk commented Apr 26, 2021

Copy link
Copy Markdown

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

@mariomc

mariomc commented Apr 27, 2021

Copy link
Copy Markdown

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

@Vincenttbk

Copy link
Copy Markdown

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

@krizzu

krizzu commented Apr 28, 2021

Copy link
Copy Markdown
Author

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.

@MystL

MystL commented Apr 28, 2021

Copy link
Copy Markdown

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.

@jnurkka

jnurkka commented Apr 28, 2021

Copy link
Copy Markdown

@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

@krizzu

krizzu commented Apr 29, 2021

Copy link
Copy Markdown
Author

@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

@1280103995

Copy link
Copy Markdown

Two years later, it's still not resolved.

@github-actions

Copy link
Copy Markdown

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 github-actions Bot added the Stale There has been a lack of activity on this issue and it may be closed soon. label Jun 14, 2023
@troZee

troZee commented Jun 14, 2023

Copy link
Copy Markdown
Contributor

Does it still valid?

@github-actions github-actions Bot removed the Stale There has been a lack of activity on this issue and it may be closed soon. label Jun 15, 2023
facebook-github-bot pushed a commit that referenced this pull request Sep 10, 2023
Summary:
This PR is a continuation of this #29471. All context you can find here:

axios/axios#2073 (comment)
axios/axios#2073 (comment)

## Changelog:

[ANDROID] [FIXED] - change `connectTimeout` to `callTimeout` in OKHttp client

Pull Request resolved: #38953

Test Plan:
- Throttle internet connection (You can use GSM one)

```
import axios from "axios";

axios.get('https://jsonplaceholder.typicode.com/users', {
  timeout: 5000
})
.then(response => {
  console.log(response.data);
})
.catch(error => {
  if (error.code === 'ECONNABORTED') {
    console.log('Request timed out');
  } else {
    console.log(error.message);
  }
});
```

**Expected result**

`Request timed out` should be visible in a console

**Videos**

Before

https://github.com/facebook/react-native/assets/12766071/03a3d666-6a27-4d67-bc68-6bcf6e908e19

After

https://github.com/facebook/react-native/assets/12766071/2650d02d-640c-43fe-b1e8-baeac78870f2

**Example URL**

Branch:
https://github.com/troZee/react-native/tree/fix/network-timeout-example

Commit:
https://github.com/troZee/react-native/commit/e255e1a423143c4bd4f3e37656a56ba2ecbbf3e2

Reviewed By: mdvacca

Differential Revision: D48414884

Pulled By: NickGerleman

fbshipit-source-id: 86324601f7459c154b5fc879f8b773df4901f6e0
@Marco29001

Copy link
Copy Markdown

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

@troZee

troZee commented Oct 3, 2023

Copy link
Copy Markdown
Contributor

This issue has been fixed here #38953. @krizzu could you close this PR?

@krizzu krizzu closed this Oct 3, 2023
@krizzu krizzu deleted the network-timeout branch October 3, 2023 13:44
@krizzu

krizzu commented Oct 3, 2023

Copy link
Copy Markdown
Author

Well done @troZee 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. p: Callstack Partner: Callstack Partner Platform: Android Android applications.

Projects

None yet

Development

Successfully merging this pull request may close these issues.