Skip to content

Bump okhttp to 4.9.0 - #1390

Merged
ferdicus merged 2 commits into
rnmapbox:masterfrom
EvanBacon:@evanbacon/android/bump-okhttp-version
Jun 11, 2021
Merged

Bump okhttp to 4.9.0#1390
ferdicus merged 2 commits into
rnmapbox:masterfrom
EvanBacon:@evanbacon/android/bump-okhttp-version

Conversation

@EvanBacon

@EvanBacon EvanBacon commented Jun 7, 2021

Copy link
Copy Markdown
Contributor

Description

Checklist

  • I have tested this on a device/simulator for each compatible OS
  • I updated the documentation yarn generate
  • I mentioned this change in CHANGELOG.md
  • I updated the typings files (index.d.ts)
  • I added/ updated a sample (/example)

Screenshot OR Video

@ferdicus ferdicus left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can confirm that the crash apparently is linked to Sdk 30+
When running on an older version 24-25 for example this isn't happening.
Hence why it was so elusive I guess :)

Anyways, thanks 🙇🏿

@ferdicus
ferdicus merged commit 358143e into rnmapbox:master Jun 11, 2021
@EvanBacon
EvanBacon deleted the @evanbacon/android/bump-okhttp-version branch June 13, 2021 02:27
@EvanBacon EvanBacon mentioned this pull request Jun 13, 2021
5 tasks
@zholmes1

Copy link
Copy Markdown

@ferdicus @EvanBacon

There is an issue with this PR that I have solved below. Please read for more info, I believe this package needs an update.

Updating my version of this package to a version which included this PR was causing this crash in HTTP requests unrelated to Mapbox.

    java.lang.NoSuchMethodError: No virtual method toString(Z)Ljava/lang/String; in class Lokhttp3/Cookie; or its super classes (declaration of 'okhttp3.Cookie' appears in /data/app/~~mcAoWfW07fJ8yS0IID_gJw==/com.myapp-E3-OLsPdksY-c2hAUjd8YQ==/base.apk!classes5.dex)
        at okhttp3.JavaNetCookieJar.saveFromResponse(JavaNetCookieJar.java:45)
        at com.facebook.react.modules.network.ReactCookieJarContainer.saveFromResponse(ReactCookieJarContainer.java:37)
        at okhttp3.internal.http.HttpHeaders.receiveHeaders(HttpHeaders.kt:207)
        at okhttp3.internal.http.BridgeInterceptor.intercept(BridgeInterceptor.kt:85)
        at okhttp3.internal.http.RealInterceptorChain.proceed(RealInterceptorChain.kt:109)
        at okhttp3.internal.http.RetryAndFollowUpInterceptor.intercept(RetryAndFollowUpInterceptor.kt:76)
        at okhttp3.internal.http.RealInterceptorChain.proceed(RealInterceptorChain.kt:109)
        at okhttp3.internal.connection.RealCall.getResponseWithInterceptorChain$okhttp(RealCall.kt:201)
        at okhttp3.internal.connection.RealCall$AsyncCall.run(RealCall.kt:517)
        at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1167)
        at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:641)
        at java.lang.Thread.run(Thread.java:923)

After some time of debugging, I found the following:

In the older okhttp3:okhttp-urlconnection versions used by other packages, JavaNetCookieJar expects okhttp3.Cookie to have a toString method that accepts a single boolean parameter. In the new version of okhttp3:okhttp (4.9.0 provided by this PR), okhttp3.Cookie.toString no longer has that boolean. Thus, the NoSuchMethodException is thrown and the app crashes.

I added the following line to my module dependencies:

implementation 'com.squareup.okhttp3:okhttp-urlconnection:4.9.0'

And the crash immediately stopped. I believe this PR should have also included this dependency to avoid conflicting versions of okhttp3:okhttp and okhttp3:okhttp-urlconnection

@ferdicus

ferdicus commented Jul 2, 2021

Copy link
Copy Markdown
Member

@zholmes1, thanks for pointing this out.
I'm not well versed in the dependency management trees of android, however as you said

causing this crash in HTTP requests unrelated to Mapbox

Why is the other Lib relying on a okhttp version pulled in by Mapbox 🤔
That feels like a shortcoming of JavaNetCookieJar to me?

Maybe I'm totally off though, so sorry bout that.

Thanks in advance

EDIT: Nevermind, you're right, I was able to reproduce this in the example project.
Thanks for the heads-up 👍🏿
Will add it as a dependency

EDIT EDIT:
Are you on the pack-in default version of the Android SDK? Or did you upgrade?
Plus, in which Android version are you observing this?

@zholmes1

zholmes1 commented Jul 2, 2021

Copy link
Copy Markdown

@ferdicus

Yes I am using the standard version of the Mapbox Android SDK that ships with this package.

I was able to produce the crash in both my emulator and my physical device, which are both running Android 11.

You can use ./gradlew app:dependencies to list the entire dependency tree for the module. I was able to confirm that there were packages that declared older versions of okhttp that were being bumped to the newer version of the okhttp package, but were still using their older declared version of the urlconnection package.

I am not extremely well versed in this kind of stuff either, to be honest. It does seem like strange behavior.

@ferdicus

ferdicus commented Jul 2, 2021

Copy link
Copy Markdown
Member

thanks for the update 👍🏿

Yeah, would be interesting if this is also happening with a newer SDK without the added dependency

@1337LutZ

1337LutZ commented Jul 8, 2021

Copy link
Copy Markdown

Also experiencing crashes after updating to this version. Adding implementation 'com.squareup.okhttp3:okhttp-urlconnection:4.9.0' fixed it straight away.

@ferdicus

ferdicus commented Jul 9, 2021

Copy link
Copy Markdown
Member

@1337LutZ are you also on the default android mapbox SDK? or did you upgrade.
I'm just trying to determine, if this should be an addition to the installation guide, or a pack in into this repo 🤔

@1337LutZ

1337LutZ commented Jul 9, 2021

Copy link
Copy Markdown

I followed the default installation guide in master branch (8.2.1) + map libre

build.gradle

// Use MapLibre https://github.com/react-native-mapbox-gl/maps/blob/master/android/install.md#using-maplibre
        rnmbglMapboxLibs = {
            implementation ("org.maplibre.gl:android-sdk:9.2.1")
            implementation ("com.mapbox.mapboxsdk:mapbox-sdk-turf:5.3.0")
        }
        rnmbglMapboxPlugins = {
            implementation ("com.mapbox.mapboxsdk:mapbox-android-gestures:0.7.0")
            implementation ("com.mapbox.mapboxsdk:mapbox-android-plugin-localization-v9:0.12.0")    {
                exclude group: 'com.mapbox.mapboxsdk', module: 'mapbox-android-sdk'
            }
            implementation ("com.mapbox.mapboxsdk:mapbox-android-plugin-annotation-v9:0.8.0")        {
                exclude group: 'com.mapbox.mapboxsdk', module: 'mapbox-android-sdk'
            }
            implementation ("com.mapbox.mapboxsdk:mapbox-android-plugin-markerview-v9:0.4.0") {
                exclude group: 'com.mapbox.mapboxsdk', module: 'mapbox-android-sdk'
            }
        }

        // Use MapLibre
        // https://github.com/react-native-mapbox-gl/maps/blob/master/android/install.md#using-maplibre
        maven {
            url = "https://dl.bintray.com/maplibre/maplibre-gl-native"
        }   

app/build.gradle
Added okhttp here to prevent breaking builds

implementation 'com.squareup.okhttp3:okhttp-urlconnection:4.9.0'

@ferdicus

ferdicus commented Jul 9, 2021

Copy link
Copy Markdown
Member

oh, ok, then you're on the maplibre flavor...
yeah, you don't even have the chance to upgrade to a newer version of that then 😅

will add it to the default dependencies then 👍🏿

@1337LutZ

1337LutZ commented Jul 9, 2021

Copy link
Copy Markdown

oh, ok, then you're on the maplibre flavor...
yeah, you don't even have the chance to upgrade to a newer version of that then 😅

will add it to the default dependencies then 👍🏿

Awesome!

mikalaiulasevich pushed a commit to OneSoil-Platform/maps that referenced this pull request Sep 23, 2021
* Bump okhttp to 4.9.0

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants