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

Fix race condition in new LazyVals #16975

Merged
merged 5 commits into from
Feb 27, 2023

Conversation

szymon-rd
Copy link
Contributor

@szymon-rd szymon-rd commented Feb 20, 2023

Resolve #16806

In the repro, Evaluating was generated as a subtype of Serializable, and the type of lazy value was erased to Serializable - these together broke the optimized condition checking if the value is initialized. For lazy val of type A we were checking if LazyValControlState <: A, and if that was not the case, we assumed it would be safe to just generate the condition _value.isInstanceOf[A]. If that condition was true in runtime, we assumed that the value was already calculated and returned it. If it was Evaluating and A =:= Serializable, then we assumed so falsely. The easiest fix is to just make the LazyValControlState <: Serializable, I will check if it doesn't affect performance.

@szymon-rd
Copy link
Contributor Author

szymon-rd commented Feb 20, 2023

I see the problem, I can't use java.io.Serializable. Actually I can, it's present in scala.js and native.

Copy link
Member

@sjrd sjrd left a comment

Choose a reason for hiding this comment

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

That was quite subtle!

Perhaps add a comment above LazyValControlState explaining why it extends Serializable?

@Kordyjan Kordyjan added this to the 3.3.0 backports milestone Feb 20, 2023
@Kordyjan Kordyjan added the backport:accepted This PR needs to be backported, once it's been backported replace this tag by "backport:done" label Feb 20, 2023
@smarter
Copy link
Member

smarter commented Feb 20, 2023

FWIW, Scala 2 uses jcstress to stress-test concurrency tests: scala/scala@3783fe8, it would be good if we could stress-test lazy vals that way too.

@szymon-rd
Copy link
Contributor Author

szymon-rd commented Feb 20, 2023

@sjrd How can I test this with scalajs? There is some linking error.

@sjrd
Copy link
Member

sjrd commented Feb 20, 2023

You cannot do it with something based on Semaphore or any other blocking stuff.

But anyway it does not matter on Scala.js, since Scala.js only uses a single thread and therefore always uses the non-thread-safe lazy val implementation. So you can skip it on Scala.js with

// scalajs: --skip

@szymon-rd szymon-rd merged commit 2aa2803 into scala:main Feb 27, 2023
Kordyjan added a commit that referenced this pull request Apr 14, 2023
@Kordyjan Kordyjan added backport:done This PR was successfully backported. and removed backport:accepted This PR needs to be backported, once it's been backported replace this tag by "backport:done" labels Apr 14, 2023
@Kordyjan Kordyjan modified the milestones: 3.3.0 backports, 3.3.1, 3.3.0 Aug 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:done This PR was successfully backported.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Scala 3.3.x Lazy Vals regression with AnyVal value ( scala.MatchError: scala.runtime.LazyVals$Evaluating )
4 participants