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.

Sign up for GitHub

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

Jump to bottom

use okhttp callTimeout to set request timeout #23038

Closed
wants to merge 1 commit into from
Closed

use okhttp callTimeout to set request timeout #23038

wants to merge 1 commit into from

Conversation

dulmandakh
Copy link
Contributor

@dulmandakh dulmandakh commented Jan 17, 2019 •
edited
Loading

OkHTTP 3.12 introduced callTimeout or full-operation timeouts. And here is the snippet from changelog (https://github.com/square/okhttp/blob/master/CHANGELOG.md)

OkHttp now offers full-operation timeouts. This sets a limit on how long the entire call may take and covers resolving DNS, connecting, writing the request body, server processing, and reading the full response body. If a call requires redirects or retries all must complete within one timeout period.

Previously we set timeout for connect, but not for full-operation. Therefore, network client connect to a server within timeout, but it may take forever to read responses or write requests. This PR fixes it.

Changelog:

[Android] [Changed] - Network request timeout works as expected using callTimeout

Test Plan:

CI is green, and everything works as before. But network timeout will for full-operation or as expected by many developers.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jan 17, 2019
@react-native-bot react-native-bot added 🌐Networking Related to a networking API. ✅Changelog labels Jan 17, 2019
@facebook-github-bot facebook-github-bot added the Import Started This pull request has been imported. This does not imply the PR has been approved. label Jan 17, 2019
facebook-github-bot
facebook-github-bot approved these changes Jan 17, 2019
View reviewed changes
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@cpojer is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@cpojer
Copy link
Contributor

cpojer commented Jan 17, 2019

Looks good!

@cpojer
Copy link
Contributor

cpojer commented Jan 17, 2019

Uh oh, I think we may not be on that version of okhttp at Facebook which makes this pretty hard to land.

@dulmandakh
Copy link
Contributor Author

@hramos would you please update okhttp at FB and merge this PR. IIRC, okhttp have no breaking changes.

@cpojer
Copy link
Contributor

cpojer commented Jan 23, 2019

@dulmandakh Unfortunately it's not that simple. It seems like we won't be able to upgrade everything depending on RN to okhttp 3.12 anytime soon (possibly months). Is there any way we could work around this?

@dulmandakh
Copy link
Contributor Author

dulmandakh commented Jan 23, 2019 via email

@cpojer
Copy link
Contributor

cpojer commented Jan 29, 2019

I asked around and there currently aren't any plans to upgrade okhttp internally. It seems that it would be a bit of work to update it for all apps. Previously it wasn't a big deal if FB apps use a different version of okhttp than RN in open source but it does matter when RN makes use of newer APIs.

I'm going to close this PR as it isn't actionable. We should revisit this whenever we update okhttp at FB.

@cpojer cpojer closed this Jan 29, 2019
@dulmandakh
Copy link
Contributor Author

how I can find out if okhttp at FB updated?

@cpojer
Copy link
Contributor

cpojer commented Jan 29, 2019

Ask me again in a few months, I'm happy to let you know :)

facebook-github-bot pushed a commit that referenced this pull request Jan 19, 2022
@facebook-github-bot
React Native sync for revisions fe905f1...51947a1
0a54819 
Summary:
This sync includes the following changes:
- **[51947a14b](facebook/react@51947a14b )**: Refactored how React/DevTools log Timeline performance data ([#23102](facebook/react#23102)) //<Brian Vaughn>//
- **[c09596cc6](facebook/react@c09596cc6 )**: Add RN_FB bundles for react-is ([#23101](facebook/react#23101)) //<Moti Zilberman>//
- **[9a7e6bf0d](facebook/react@9a7e6bf0d )**: Add --no-show-signature to "git show" commands ([#23038](facebook/react#23038)) //<Stefan Sundin>//
- **[2f26eb85d](facebook/react@2f26eb85d )**: Add exports field to react-refresh's package.json ([#23087](facebook/react#23087)) //<Gray Zhang>//
- **[811634762](facebook/react@811634762 )**: add enableTransitionTracing feature flag ([#23079](facebook/react#23079)) //<Luna Ruan>//

Changelog:
[General][Changed] - React Native sync for revisions fe905f1...51947a1

jest_e2e[run_all_tests]

Reviewed By: rickhanlonii, kacieb

Differential Revision: D33634332

fbshipit-source-id: a83b663a122a2cb79225ca33a007fe1774728c03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@facebook-github-bot facebook-github-bot facebook-github-bot approved these changes

@EgidioCaprino EgidioCaprino EgidioCaprino approved these changes

@cpojer cpojer Awaiting requested review from cpojer

@hramos hramos Awaiting requested review from hramos

Assignees
No one assigned
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Import Started This pull request has been imported. This does not imply the PR has been approved. 🌐Networking Related to a networking API.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@dulmandakh @cpojer @EgidioCaprino @facebook-github-bot @react-native-bot