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.

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

Remove usage of toDuration() #7329

Merged
merged 1 commit into from Jun 15, 2022
Merged

Remove usage of toDuration() #7329

merged 1 commit into from Jun 15, 2022

Conversation

staktrace
Copy link
Contributor

The toDuration() function was only introduced in Kotlin 1.6. If using
okhttp3 as part of a gradle 7.4.2 plugin it will run with Kotlin 1.5.31
in the runtime classpath, which results in a NoSuchMethodError.

The toDuration() function was only introduced in Kotlin 1.6. If using
okhttp3 as part of a gradle 7.4.2 plugin it will run with Kotlin 1.5.31
in the runtime classpath, which results in a NoSuchMethodError.
@staktrace staktrace changed the title Remove usage to toDuration() Remove usage of toDuration() Jun 13, 2022
@staktrace
Copy link
Contributor Author

So this is failing a test that is annotated as @Flaky. The test in question is not failing for me locally, so presumably it's failing because of flakiness.

@staktrace
Copy link
Contributor Author

I'm also unable to re-run the test jobs (presumably not enough permissions on this repo) so I'm kinda stuck here. Any help would be appreciated. @yschimke @swankjesse

@yschimke
Copy link
Collaborator

No problem, there are some flaky tests. But because of some choices we can't rerun them automatically.

I'll land tomorrow if no other reviews.

@yschimke yschimke merged commit 2c93710 into square:master Jun 15, 2022
@staktrace
Copy link
Contributor Author

Thank you! I've confirmed that the latest 5.0.0-SNAPSHOT release doesn't have the problem any more. Would it be possible to push a new alpha.9 release with the fix?

@yschimke
Copy link
Collaborator

cc @swankjesse

BTW since I struggled to test this, I really appreciate you following through with this and confirming it. Thanks!

@yschimke
Copy link
Collaborator

thanks @swankjesse 5.0.0-alpha.9 is available.

@staktrace
Copy link
Contributor Author

So unfortunately this didn't work as intended... the 5.0.0-SNAPSHOT build still works without any issues, but when I switch to 5.0.0-alpha.9, the test in question fails with Caused by: java.lang.NoSuchMethodError: 'double kotlin.time.Duration$Companion.convert(double, kotlin.time.DurationUnit, kotlin.time.DurationUnit)'.

I don't understand why the snapshot build is different in this respect to alpha.9. I compared the gradle build scans from the two runs and looked at the differences - other than the okhttp version the only other change was okio version 3.0.0 (with SNAPSHOT) went to 3.1.0 (with alpha.9). Applying that change manually to the SNAPSHOT setup still didn't make any difference. However, this seems to imply that the -SNAPSHOT build being used is an old one, prior to when okio was bumped in #7247. I'm not sure why that would be the case.

@staktrace
Copy link
Contributor Author

Oho! So some more digging produced the discovery that I need to set https://s01.oss.sonatype.org/content/repositories/snapshots as the snapshots repository, rather than https://oss.sonatype.org/content/repositories/snapshots. I had assumed the latter would redirect to the former (or perhaps to one of a pool of load-balanced servers) but apparently they are distinct. In particular the 5.0.0-SNAPSHOT version on the latter is 20211217.184251-6 while the s01 server has 20220616.185128-116. So when I tested with SNAPSHOT I was getting a stale snapshot build.

@yschimke
Copy link
Collaborator

So working now?

@staktrace
Copy link
Contributor Author

No, unfortunately. I'm still getting the NoSuchMethodError: 'double kotlin.time.Duration$Companion.convert(double, kotlin.time.DurationUnit, kotlin.time.DurationUnit)'. Only difference is I can consistently reproduce it with the alpha.9 and the SNAPSHOT (and even with local builds) now. At this point my best guess is that it has something to do with the @ExperimentalTime annotation that is on the function in 1.6.21 but wasn't on the function in 1.5.31.

@staktrace
Copy link
Contributor Author

I took the kotlin-stdlib-1.5.31.jar file, extracted the kotlin/time/Duration$Companion.class file, and ran it through javap -v. This showed me signature of the function is this:

  public final double convert(double, java.util.concurrent.TimeUnit, java.util.concurrent.TimeUnit);

So in the kotlin 1.5.31 runtime stdlib, the function signature uses java.util.concurrent.TimeUnit instead of kotlin.time.DurationUnit, but okhttp3 is compiled against 1.6.21 and so is looking for a function with kotlin.time.DurationUnit. This whole experimental time API is a bit of a shitshow...

@JakeWharton
Copy link
Member

This whole Gradle-ships-an-old-ass-version-of-Kotlin-on-the-buildscript-classpath-preventing-normal-dependency-resolution-semantics is a bit of a shit show.

Let's place blame where blame is due.

@yschimke
Copy link
Collaborator

I guess I don't see a serious problem here.

For JVM - you can use the TimeUnit forms of these methods.
For KMP - You probably are using Kotlin 1.6, so not a problem.

Is there a way we can mark methods as only being Kotlin 1.6?

staktrace added a commit to staktrace/okhttp that referenced this pull request Jun 20, 2022
This reverts commit 2c93710, as
it doesn't actually solve the problem it was intending to solve.
staktrace added a commit to staktrace/okhttp that referenced this pull request Jun 20, 2022
Based on the discovery in square#7329 (comment)
this commit tries a different approach to make okhttp compatible
with the kotlin 1.5 runtime. Specifically, it converts the DurationUnit
instance to a java.util.concurrent.TimeUnit instance and uses the
existing codepath to handle it. The conversion must be done without
using `DurationUnit.toTimeUnit()` since that is again a function
that would not exist in the 1.5 kotlin runtime environment.
@staktrace
Copy link
Contributor Author

I'm not super familiar with multiplatform Kotlin stuff, but if I understood what you're saying correctly we need to modify the jvm-specific codepath. I put up a PR at #7343 that seems to work for me.

@yschimke
Copy link
Collaborator

the PR looks good.

But what I meant was that you can avoid calling this code path in gradle apps. But then I found your original stacktrace, which shows it isn't your code. So I was wrong.

Caused by: java.lang.NoSuchMethodError: 'long kotlin.time.DurationKt.toDuration(int, kotlin.time.DurationUnit)'
        at okhttp3.internal._CacheControlCommonKt.commonMaxStale(-CacheControlCommon.kt:59)
        at okhttp3.CacheControl$Builder.maxStale(CacheControl.kt:158)
        at okhttp3.internal._CacheControlCommonKt.commonForceCache(-CacheControlCommon.kt:83)
        at okhttp3.CacheControl.<clinit>(CacheControl.kt:210)

I think your PR looks like the right way to go.

My alternative would be moving this code to expect/actuals

.maxStale(Int.MAX_VALUE, DurationUnit.SECONDS)

yschimke pushed a commit that referenced this pull request Jun 21, 2022
* Revert "Remove usage of toDuration() (#7329)"

This reverts commit 2c93710, as
it doesn't actually solve the problem it was intending to solve.

* Make jvm functions compatible with kotlin 1.5 runtime

Based on the discovery in #7329 (comment)
this commit tries a different approach to make okhttp compatible
with the kotlin 1.5 runtime. Specifically, it converts the DurationUnit
instance to a java.util.concurrent.TimeUnit instance and uses the
existing codepath to handle it. The conversion must be done without
using `DurationUnit.toTimeUnit()` since that is again a function
that would not exist in the 1.5 kotlin runtime environment.
yschimke added a commit to yschimke/okhttp that referenced this pull request Jul 23, 2022
yschimke added a commit that referenced this pull request Jul 25, 2022
* Revert "Downgrade to kotlin apiVersion 1.4 (#7267)"
* Revert "Improve runtime compatibility with kotlin 1.5.31 (#7343)"
* Revert "Remove usage of toDuration() (#7329)"
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.

None yet

3 participants