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

Native SynchronizedObject actualization split between Apple and Non-Apple source sets breaks dependent projects #520

Open
fzhinkin opened this issue Feb 26, 2025 · 21 comments
Labels
bug major An important problem that will require some effort

Comments

@fzhinkin
Copy link
Contributor

#499 split once single SynchronizedObject actualization between two source sets, apple and non-apple. On native, SynchronizedObject actualization provides three additional functions (tryLock, lock, unlock) and some projects, like kotlinx-coroutines use these functions in their corresponding native source sets.

After SynchronizedObject being split in two identical (in terms of public ABI) implementations in two different source sets, it is no longer possible to use those three functions from a common native source sets as they are only available in corresponding apple and native-non-apple source sets.

This is a regression and we need to continue providing tryLock, lock, unlock functions in native source set.

@fzhinkin fzhinkin added bug major An important problem that will require some effort labels Feb 26, 2025
@fzhinkin
Copy link
Contributor Author

For now, we at least have to rollback #499 from kotlin-community/dev branches to unblock Kotlin release trains.

@fzhinkin
Copy link
Contributor Author

On the coroutines side, compilation fails with:

> Task :kotlinx-coroutines-core:compileNativeMainKotlinMetadata FAILED
e: file:///...../kotlinx.coroutines/kotlinx-coroutines-core/native/src/internal/Concurrent.kt:7:27 'actual typealias ReentrantLock = SynchronizedObject' has no corresponding members for expected class members:

    expect fun tryLock(): Boolean

    expect fun unlock(): Unit
...

@qwwdfsad
Copy link
Collaborator

qwwdfsad commented Feb 26, 2025

That is suspicious because I've explicitly tested and benchmarked #499 as part of the Kotlin/kotlinx.coroutines#4350

Could it be the case that #512 is the reason for the issue?

@fzhinkin
Copy link
Contributor Author

@qwwdfsad, reverting only #512 does not solve the issue, so I don't think so.

@qwwdfsad
Copy link
Collaborator

qwwdfsad commented Feb 26, 2025

It seems to be non-apple specific, probably reproducible on Linux.

I've built the current develop and kotlinx-coroutines-core successfully consumes it (on my OS X machine)

@qwwdfsad
Copy link
Collaborator

Update:
./gradlew :kotlinx-coroutines-core:macosArm64BenchmarkBenchmark works and properly reflects updates in afu version, but ./gradlew publishToMavenLocal fails

@fzhinkin
Copy link
Contributor Author

Steps to reproduce:

  • Build and publish kotlinx-atomicfu from develop branch locally:
./gradlew clean publishToMavenLocal
  • Compile native metadata for kotlinx-coroutines's core module at kotlin-community/k2/dev branch using that local AFU publication:
 ./gradlew clean :kotlinx-coroutines-core:compileNativeMainKotlinMetadata  -Pbuild_snapshot_train=true  -Pkotlin_repo_url=https://packages.jetbrains.team/maven/p/kt/dev -Patomicfu_version=0.27.0-SNAPSHOT -Pkotlin_version=2.2.0-dev-7255 -Pkotlin_snapshot_version=2.2.0-dev-7255  -Pskip_snapshot_checks=true
  • Get the error:
> Task :kotlinx-coroutines-core:compileNativeMainKotlinMetadata FAILED
e: file:///Users/filipp.zhinkin/Development/kotlinx.coroutines/kotlinx-coroutines-core/native/src/internal/Concurrent.kt:7:27 'actual typealias ReentrantLock = SynchronizedObject' has no corresponding members for expected class members:

    expect fun tryLock(): Boolean

    expect fun unlock(): Unit

e: file:///Users/filipp.zhinkin/Development/kotlinx.coroutines/kotlinx-coroutines-core/native/src/internal/Concurrent.kt:9:82 Cannot infer type for this parameter. Specify it explicitly.
e: file:///Users/filipp.zhinkin/Development/kotlinx.coroutines/kotlinx-coroutines-core/native/src/internal/Concurrent.kt:9:82 Unresolved reference. None of the following candidates is applicable because of a receiver type mismatch:
expect fun <T> ReentrantLock.withLock(block: () -> T): T
e: file:///Users/filipp.zhinkin/Development/kotlinx.coroutines/kotlinx-coroutines-core/native/src/internal/Synchronized.kt:17:99 Cannot infer type for this parameter. Specify it explicitly.
e: file:///Users/filipp.zhinkin/Development/kotlinx.coroutines/kotlinx-coroutines-core/native/src/internal/Synchronized.kt:17:99 Unresolved reference. None of the following candidates is applicable because of a receiver type mismatch:
expect fun <T> ReentrantLock.withLock(block: () -> T): T

@fzhinkin
Copy link
Contributor Author

@stefanhaustein could you please take a look at the issue? It slipped through our CI at the time #499 was reviewed and merged.

I didn't thought a possible fix through, but it seems like we need to continue providing a single SynchonizedObject actualization in the common native source set (as it was before #499) and move all new code introduced in #499 into separate classes (or free standing functions, IDK) implemented in a more specific source sets.

@stefanhaustein
Copy link

stefanhaustein commented Feb 27, 2025

I'll look into it -- I am assuming it's not super urgent as you are already rolling back?

PS: It would be great if there would be a more straightforward test catching this...

@fzhinkin
Copy link
Contributor Author

I'll look into it -- I am assuming it's not super urgent as you are already rolling back?

We rolled backed changes in a branch used in Kotlin release trains, but AFU's develop branch remained untouched. It will become a blocker for us once we decide to release a new AFU version, but I don't think it'll happen tomorrow or a next week, so it's not extremely urgent.

@JakeWharton
Copy link

Klib API dumping is not enabled for this project. That should have caught this, right?

fzhinkin added a commit that referenced this issue Feb 27, 2025
@fzhinkin
Copy link
Contributor Author

@fzhinkin
Copy link
Contributor Author

Klib API dumping is not enabled for this project. That should have caught this, right?

It's worth checking, I doubt it can catch this particular issue as the ABI should have remained the same across all the targets. It's sources being moved between source sets what cause compilation issues.

@fzhinkin
Copy link
Contributor Author

I checked: changes in the dumped Klib ABI give zero clues on this particular problem.

@stefanhaustein
Copy link

Thanks! I was assuming rolling back meant rolling back "everywhere". I look into addressing this tomorrow (= Friday). It might be feasible to address this by sharing more of the implementation.

@stefanhaustein
Copy link

Update: This should be addressed in #517 now...

@stefanhaustein
Copy link

PS Vesevold told me he is still seeing the problem, but the SynchronizedObject source is no longer split and I did not see the new test fail on #517... If anybody has ideas / suggestions, please don't hesitate to comment O:)

@fzhinkin
Copy link
Contributor Author

I can't tell exactly what problem is Vsevolod seeing, but for me the regression test is passing, as well as kotlinx-coroutines compilation that uses local atomicfu build compiled from #517 sources.

@stefanhaustein
Copy link

@fzhinkin would you mind sharing your patch for building kotlinx-coroutines with atomicfu from maven-local?

@fzhinkin
Copy link
Contributor Author

fzhinkin commented Mar 3, 2025

@stefanhaustein I ended up building it w/o any patches using commands from #520 (comment)

@stefanhaustein
Copy link

Thanks, that worked for me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug major An important problem that will require some effort
Projects
Status: No status
Development

No branches or pull requests

4 participants