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

Remove okhttp internal util usage #37843

Closed
wants to merge 3 commits into from
Closed

Remove okhttp internal util usage #37843

wants to merge 3 commits into from

Conversation

adrianha
Copy link
Contributor

@adrianha adrianha commented Jun 13, 2023 •
edited
Loading

Summary:

Remove okhttp internal Util usage so it can be compatible with okhttp 5.0.0 alpha.

Changelog:

[ANDROID] [CHANGED] - Remove okhttp3 internal util usage

Test Plan:

None, the implementation is based on okhttp internal util

@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 Jun 13, 2023
@javache
Copy link
Member

javache commented Jun 13, 2023

Do we know if this has the same behaviour as before, where checked exceptions are ignored? https://github.com/square/okhttp/blob/8c8c3dbcfa91e28de2e13975ec414e07f153fde4/okhttp/src/commonMain/kotlin/okhttp3/internal/-UtilCommon.kt#LL282C1-L288C2

cortinico
cortinico requested changes Jun 13, 2023
View reviewed changes
...ct-native/ReactAndroid/src/main/java/com/facebook/react/modules/network/RequestBodyUtil.java Outdated
@@ -152,7 +151,7 @@ public void writeTo(BufferedSink sink) throws IOException {
source = Okio.source(inputStream);
sink.writeAll(source);
} finally {
Util.closeQuietly(source);
source.close();
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we copy over the code of closeQuitely, namely:

fun Closeable.closeQuietly() {
  try {
    close()
  } catch (rethrown: RuntimeException) {
    throw rethrown
  } catch (_: Exception) {
  }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks for the suggestion, i’ll update it later

@adrianha
Copy link
Contributor Author

Source exception

just added the IOException catch block here, I think it should be enough to silence the exception as Source only throws IOException. Any thoughts @javache @cortinico?

@cortinico
Copy link
Contributor

just added the IOException catch block here, I think it should be enough to silence the exception as Source only throws IOException. Any thoughts @javache @cortinico?

I would suggest you just copy over the same code as closeQuietly (I would also add a comment to where the code was vendored from).

You're right that close only fires IOException but, as users might swap different Java & OkHTTP version, the exception types could differ in the future.

@adrianha adrianha requested a review from cortinico June 14, 2023 15:52
@cortinico
Copy link
Contributor

/rebase

@analysis-bot
Copy link

analysis-bot commented Jun 14, 2023 •
edited
Loading

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 8,757,538 -146
android hermes armeabi-v7a 8,070,118 -140
android hermes x86 9,250,139 -142
android hermes x86_64 9,099,283 -135
android jsc arm64-v8a 9,318,912 +80
android jsc armeabi-v7a 8,508,818 +78
android jsc x86 9,382,411 +91
android jsc x86_64 9,635,644 +76

Base commit: 8c9e375
Branch: main

cortinico
cortinico requested changes Jun 14, 2023
View reviewed changes
Copy link
Contributor

@cortinico cortinico left a comment

Choose a reason for hiding this comment

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

See my previous comment

@adrianha adrianha requested a review from cortinico June 15, 2023 03:35
@facebook-github-bot
Copy link
Contributor

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

1 similar comment
@facebook-github-bot
Copy link
Contributor

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

@adrianha
Copy link
Contributor Author

thank you for reviewing @cortinico, any actionables from my side?

@cortinico
Copy link
Contributor

Not for now, we'll be merging this internally. It can take up to a couple of days

@adrianha
Copy link
Contributor Author

great, thanks!

@facebook-github-bot
Copy link
Contributor

@cortinico merged this pull request in 3e30326.

@facebook-github-bot facebook-github-bot added the Merged This PR has been merged. label Jun 15, 2023
adamaveray added a commit to adamaveray/react-native that referenced this pull request Jun 16, 2023
@adamaveray
Merge branch 'main' into feature/automaticKeyboardInsetsImprovements
9d7205f 
* main: (135 commits)
  translation auto-update for i18n/twilight.config.json on master
  Interop: Introduce Bridge proxy
  Remove okhttp internal util usage (facebook#37843)
  Update debian to fix CI while updating Node (facebook#37841)
  fix: foreground ripple crash on api < 23 (facebook#37901)
  Re-add the top level LICENSE file (facebook#37916)
  Deploy 0.209.0 to xplat (facebook#37921)
  Re-enable direct debugging with JSC on iOS 16.4+ (facebook#37914)
  add emitObjectProp in parser primitives (facebook#37904)
  Make React-utils its own pod (facebook#37659)
  feat: allow custom assignment of rootView to rootViewController (facebook#37873)
  Switch xplat prettier config to hermes plugin (facebook#37915)
  Set iOS AppState to inactive when app is launching (facebook#37690)
  Use `fileExists` in replace_hermes script (facebook#37911)
  (docs): fix license url (facebook#37909)
  Revert D46719890: Re-enable direct debugging with JSC on iOS 16.4+
  Re-enable direct debugging with JSC on iOS 16.4+ (facebook#37874)
  Fix component type references in xplat (facebook#37903)
  Remove usage of passthroughAnimatedPropExplicitValues in ScrollViewStickyHeader (facebook#37867)
  test runtime lifecycle callback (facebook#37897)
  ...
@adrianha adrianha deleted the remove-okhttp-internal branch June 29, 2023 23:42
kelset pushed a commit that referenced this pull request Jul 10, 2023
@adrianha @kelset
Remove okhttp internal util usage (#37843)
03b9b52 
Summary:
Remove okhttp internal `Util` usage so it can be compatible with [okhttp 5.0.0 alpha](https://square.github.io/okhttp/changelogs/changelog/#version-500-alpha7).

## Changelog:

<!-- Help reviewers and the release process by writing your own changelog entry.

Pick one each for the category and type tags:

For more details, see:
https://reactnative.dev/contributing/changelogs-in-pull-requests
-->

[ANDROID] [CHANGED] - Remove okhttp3 internal util usage

Pull Request resolved: #37843

Test Plan: None, the implementation is based on okhttp internal util

Reviewed By: NickGerleman

Differential Revision: D46764363

Pulled By: cortinico

fbshipit-source-id: e46770625f19057fa7374be15e92903d7966f880
Kudo pushed a commit to expo/react-native that referenced this pull request Jul 11, 2023
@adrianha @Kudo
Remove okhttp internal util usage (facebook#37843)
e2ac2da 
Summary:
Remove okhttp internal `Util` usage so it can be compatible with [okhttp 5.0.0 alpha](https://square.github.io/okhttp/changelogs/changelog/#version-500-alpha7).

## Changelog:

<!-- Help reviewers and the release process by writing your own changelog entry.

Pick one each for the category and type tags:

For more details, see:
https://reactnative.dev/contributing/changelogs-in-pull-requests
-->

[ANDROID] [CHANGED] - Remove okhttp3 internal util usage

Pull Request resolved: facebook#37843

Test Plan: None, the implementation is based on okhttp internal util

Reviewed By: NickGerleman

Differential Revision: D46764363

Pulled By: cortinico

fbshipit-source-id: e46770625f19057fa7374be15e92903d7966f880
This was referenced Aug 1, 2023
Open
Open
This was referenced Aug 10, 2023
Closed
Merged
@cipolleschi cipolleschi mentioned this pull request Sep 21, 2023
Merged
@cipolleschi cipolleschi mentioned this pull request Oct 12, 2023
Merged
@cipolleschi cipolleschi mentioned this pull request Nov 1, 2023
Merged
@stevenpetryk stevenpetryk mentioned this pull request Nov 28, 2023
Merged
@cipolleschi cipolleschi mentioned this pull request Dec 27, 2023
Merged
This was referenced Jan 4, 2024
Merged
Merged
Merged
Closed
This was referenced Feb 28, 2024
Merged
Merged
Merged
@cipolleschi cipolleschi mentioned this pull request Jun 28, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cortinico cortinico cortinico approved these changes

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. Merged This PR has been merged.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@adrianha @javache @cortinico @analysis-bot @facebook-github-bot