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

Data race in EventLoop #3251

Closed
shmuelr opened this issue Apr 13, 2022 · 4 comments
Closed

Data race in EventLoop #3251

shmuelr opened this issue Apr 13, 2022 · 4 comments
Assignees
Labels

Comments

@shmuelr
Copy link
Contributor

shmuelr commented Apr 13, 2022

Hey there,

Similar to #2660, our internal Java TSAN tests in Google picked up a race in EventLoop.common.kt

Write of size 4 at
kotlinx.coroutines.EventLoopImplBase$DelayedTask.setHeap(Lkotlinx/coroutines/internal/ThreadSafeHeap;)V EventLoop.common.kt:419
versus read of size 4 at
kotlinx.coroutines.EventLoopImplBase$DelayedTask.dispose()V EventLoop.common.kt:480

The race in EventLoop seems to be here
https://github.com/Kotlin/kotlinx.coroutines/blob/master/kotlinx-coroutines-core/common/src/EventLoop.common.kt#L417-L420
that write is not thread safe, but the dispose function is marked as synchronized
https://github.com/Kotlin/kotlinx.coroutines/blob/master/kotlinx-coroutines-core/common/src/EventLoop.common.kt#L479-L485
leading to the race in _heap reading & writing.

Is this race safe?

Thanks!

@qwwdfsad qwwdfsad self-assigned this May 16, 2022
@qwwdfsad
Copy link
Member

qwwdfsad commented May 16, 2022

Could you please provide more detailed stacktraces? E.g. on which code path exactly setHeap is racing

@qwwdfsad
Copy link
Member

Oh, I see, both dispose and setHeap are synchronized, but they actually use different monitors.

This is definitely a bug, I already can see an interleaving that leads to an error, working on the fix.
I must admit that TSAN testing is amazingly valuable as such bugs are really hard to catch with non-targeting stress test or code-review

@qwwdfsad qwwdfsad added the bug label May 16, 2022
@shmuelr
Copy link
Contributor Author

shmuelr commented May 16, 2022

Nice! Thanks for working on a fix, I'm happy we were able to help here :D

@LifeIsStrange
Copy link

@shmuelr Hi :) What is this Java/kotlin TSAN you're talking about??
I have seen the openjdk TSAN JEP here:
https://bugs.openjdk.java.net/browse/JDK-8208520
https://openjdk.java.net/projects/tsan/

However the implementation still seems to be a work in progress and has not received updates since 2019? :/
How to use it, does the flag work on default openjdk or do I need a special OpenJDK build? Is there kotlin support out of the box?
Or do you have a custom solution at google?
I'm looking forward to your answer as the OpenJDK ecosystem would strongly benefit from a TSAN tool.

pablobaxter pushed a commit to pablobaxter/kotlinx.coroutines that referenced this issue Sep 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants