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

Random.javaUtilConcurrentThreadLocalRandom not thread-local #2766

Closed
durban opened this issue Jan 20, 2022 · 6 comments
Closed

Random.javaUtilConcurrentThreadLocalRandom not thread-local #2766

durban opened this issue Jan 20, 2022 · 6 comments

Comments

@durban
Copy link
Contributor

durban commented Jan 20, 2022

I think that cats.effect.std.Random.javaUtilConcurrentThreadLocalRandom might call a ThreadLocalRandom from the incorrect thread. Its unlikely this would cause any problems on OpenJDK, since there every ThreadLocalRandom is the "same", but... it's still not allowed, and who knows what other JVM-s might do.

I think this could happen in the following way. Inlining a few things, what it does is like this:

def nextBoolean: F[Boolean] =
  for {
    r <- Sync[F].delay(new SRandom(java.util.concurrent.ThreadLocalRandom.current()))
    out <- Sync[F].delay(r.nextBoolean())
  } yield out

So there is a flatMap between the call to .current() and (for example) the call to .nextBoolean(). And if there is an "autocede" between them, the fiber could be rescheduled to another JVM thread. And then .nextBoolean() would be called on another thread's ThreadLocalRandom. At least I think this could happen.

@vasilmkd
Copy link
Member

That is a valid point and should be fixed. Feel free to open a PR.

@djspiewak
Copy link
Member

djspiewak commented Jan 22, 2022

This is actually a great first issue. Fixable just by collapsing the two delay blocks together. Please target the series/3.3.x branch!

@PiotrBosak
Copy link

I allowed myself to try to fix the issue. I hope that's fine with @durban .

@durban
Copy link
Contributor Author

durban commented Jan 29, 2022

@PiotrBosak Sure, absolutely :-)

@armanbilge
Copy link
Member

Btw this would have been another possible application of #2610.

@durban
Copy link
Contributor Author

durban commented Mar 1, 2022

This was fixed by #2784.

@durban durban closed this as completed Mar 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants