Skip to content

Conversation

@troZee
Copy link
Contributor

@troZee troZee commented Aug 11, 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

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

Before.mov

After

After.mov

Example URL

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

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

@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. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. labels Aug 11, 2023
@analysis-bot
Copy link

analysis-bot commented Aug 11, 2023

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 8,947,045 +3
android hermes armeabi-v7a n/a --
android hermes x86 n/a --
android hermes x86_64 n/a --
android jsc arm64-v8a 9,540,850 +44
android jsc armeabi-v7a n/a --
android jsc x86 n/a --
android jsc x86_64 n/a --

Base commit: ae88aef
Branch: main

@cipolleschi cipolleschi added Partner p: Callstack Partner: Callstack labels Aug 11, 2023
@NickGerleman
Copy link
Contributor

I looked through to see what is piping timeout to native, and confirmed new behavior is the expectation for XMLHttpRequest timeout prop (fetch just polyfills that). So, change LGTM.

@facebook-github-bot
Copy link
Contributor

@NickGerleman has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

// client and set the timeout explicitly on the clone. This is cheap as everything else is
// shared under the hood.
// See https://github.com/square/okhttp/wiki/Recipes#per-call-configuration for more information
if (timeout != mClient.connectTimeoutMillis()) {
Copy link
Contributor

@NickGerleman NickGerleman Aug 16, 2023

Choose a reason for hiding this comment

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

This line is no longer correct (should be callTimeoutMillis()). I can fix it up quickly.

Choose a reason for hiding this comment

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

how change this line?

@NickGerleman
Copy link
Contributor

It looks like RN is internally built using okhttp 3.6.0, which is way behind OSS, and doesn't yet have these APIs (they are introduced in 3.12.0). I will take a look at how painful it would be to bump this dependency in internal apps.

@troZee
Copy link
Contributor Author

troZee commented Aug 19, 2023

Hey 👋
Thank you @NickGerleman for checking that issue. Could we plan a migration from 3.6.0 to 3.12.0? Could you estimate, if this migration is complex to do? Maybe we should consider creating a separate task to bump the oss version and the internal one to https://github.com/square/okhttp/tree/parent-3.14.9 ?

Moreover, https://square.github.io/okhttp/changelogs/changelog_3x/#version-3149 has 2 years and maybe we should consider migrating it to the newest one https://square.github.io/okhttp/changelogs/changelog_4x/#version-4100 ?

@github-actions
Copy link

This pull request was successfully merged by @troZee in e00f244.

When will my fix make it into a release? | Upcoming Releases

@github-actions github-actions bot added the Merged This PR has been merged. label Sep 10, 2023
@troZee troZee deleted the fix/network-timeout branch October 3, 2023 13:13
@hasan642
Copy link

I changed the lines you mentioned and still not working!
@NickGerleman

is working with you? @Marco29001

@Marco29001
Copy link

I tested this with a pre-release version and if the timeout issue mentioned in this PR works, but there is still an issue that I think is related to this issue, what happens is that if the connection is lost for a long time when I recover it, it happens to me. I can't make a new request until a certain amount of time passes, this only happens on Android
@hasan642

@hasan642
Copy link

@Marco29001 we need a solution please
Axios android duplicates request to 5 times .

@troZee
Copy link
Contributor Author

troZee commented Nov 23, 2023

@Marco29001 @hasan642 could you provide a repro for that?
Here I could not reproduce that

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. Merged This PR has been merged. p: Callstack Partner: Callstack Partner Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants