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

Improve runtime compatibility with kotlin 1.5.31 #7343

Merged
merged 2 commits into from Jun 21, 2022

Conversation

staktrace
Copy link
Contributor

@staktrace staktrace commented Jun 20, 2022

I tried to fix this in #7329 but that didn't work. This reverts that attempt and applies a different fix that seems to work better. See individual commit messages for details.

staktrace added 2 commits Jun 20, 2022
This reverts commit 2c93710, as
it doesn't actually solve the problem it was intending to solve.
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.
@@ -50,20 +50,20 @@ internal fun CacheControl.commonToString(): String {

internal fun CacheControl.Builder.commonMaxAge(maxAge: Int, timeUnit: DurationUnit) = apply {
require(maxAge >= 0) { "maxAge < 0: $maxAge" }
val maxAgeSecondsDouble = Duration.convert(maxAge.toDouble(), timeUnit, DurationUnit.SECONDS)
this.maxAgeSeconds = maxAgeSecondsDouble.commonClampToInt()
val maxAgeSecondsLong = maxAge.toDuration(timeUnit).inWholeSeconds
Copy link
Collaborator

@yschimke yschimke Jun 20, 2022

Choose a reason for hiding this comment

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

Nice, so this is reverting back to before these PRs.

// to TimeUnit (as it was pre-1.6) and where it is a distinct wrapper enum (in 1.6). We also
// can't use durationUnit.toTimeUnit() because that doesn't exist in the typealiased case.
// This function should work in either case.
internal fun toJavaTimeUnit(durationUnit: DurationUnit) = TimeUnit.valueOf(durationUnit.name)
Copy link
Collaborator

@yschimke yschimke Jun 20, 2022

Choose a reason for hiding this comment

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

This doesn't look to bad, a lookup in a Hashmap, from a constant name (at least on JVM).

https://github.com/frohoff/jdk8u-dev-jdk/blob/da0da73ab82ed714dc5be94acd2f0d00fbdfe2e9/src/share/classes/java/lang/Class.java#L3339

@yschimke yschimke merged commit 3ff1b61 into square:master Jun 21, 2022
15 of 17 checks passed
@staktrace
Copy link
Contributor Author

staktrace commented Jun 27, 2022

Confirmed that (finally!) this fixed the issue we were having.

@staktrace staktrace deleted the oldkotlin2 branch Jun 27, 2022
@yschimke
Copy link
Collaborator

yschimke commented Jun 27, 2022

@staktrace a big hearty thanks for your efforts

@staktrace
Copy link
Contributor Author

staktrace commented Jun 28, 2022

Here we go again! This PR fixed the issue we were having, but there's a new (but I guess) related issue.

Caused by: java.lang.NoClassDefFoundError: kotlin/time/DurationUnit
	at okhttp3.internal._CacheControlCommonKt.commonForceCache(-CacheControlCommon.kt:83)
	at okhttp3.CacheControl.<clinit>(CacheControl.kt:214)
	at okhttp3.Request.cacheControl(Request.kt:115)
	at okhttp3.internal.cache.CacheStrategy$Factory.compute(CacheStrategy.kt:134)
	at okhttp3.internal.cache.CacheInterceptor.intercept(CacheInterceptor.kt:52)
[snip]
Caused by: java.lang.ClassNotFoundException: kotlin.time.DurationUnit

This one didn't get caught by the tests for our plugin (which is the thing that uses okhttp3) but started failing in repos that used the plugin. I'll try to construct a minimal test case for this that ideally we can check in, although who knows if there will be more problems after this that the test case doesn't capture.

@staktrace
Copy link
Contributor Author

staktrace commented Jun 28, 2022

I made a standalone reproducer and filed #7361

yschimke added a commit to yschimke/okhttp that referenced this issue Jul 23, 2022
yschimke added a commit that referenced this issue 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
Development

Successfully merging this pull request may close these issues.

None yet

2 participants