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

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

Closed
soronpo opened this issue Feb 2, 2023 · 17 comments · Fixed by #16975
Assignees
Labels
area:transform itype:bug regression This worked in a previous version but doesn't anymore stat:needs minimization Needs a self contained minimization
Milestone

Comments

@soronpo
Copy link
Contributor

soronpo commented Feb 2, 2023

I decided to report this before I find the time to minimize, because this is a severe issue for anyone using AnyVal values.
They should avoid upgrading to 3.3.x until this is found and resolved.

Compiler version

v3.3.0-RC2

Minimized code

I still don't have a minimized error and the errors are occurring randomly, so it makes it very problematic to minimize.
In general where it fails is that it tries to match against the underlying value of the AnyVal, and it gets a MatchError.

TBD

Output

 scala.MatchError: scala.MatchError: scala.runtime.LazyVals$Evaluating$@6ce08dec (of class scala.runtime.LazyVals$Evaluating$) (of class scala.MatchError)

Expectation

No error.

@soronpo soronpo added itype:bug stat:needs minimization Needs a self contained minimization stat:needs triage Every issue needs to have an "area" and "itype" label regression This worked in a previous version but doesn't anymore labels Feb 2, 2023
@sjrd sjrd added this to the 3.3.0 backports milestone Feb 2, 2023
@szymon-rd
Copy link
Contributor

I would be really grateful if you could provide some minimization, even if it fails only a percentage of time.

@soronpo
Copy link
Contributor Author

soronpo commented Feb 2, 2023

I have it failing quite consistently but in different stack traces (the final part is the same).
I guess that it might also be because of a concurrency issue, and only when I run multiple tests then it fails.
I'll try to force tests to run sequentially and see if indeed that's the case.

@szymon-rd
Copy link
Contributor

szymon-rd commented Feb 2, 2023

This snippet works ok, for an example:

class LazyHolder {
    lazy val xInstant: AnyVal = {
        21
    }
}

val ec = Executors.newFixedThreadPool(Runtime.getRuntime().availableProcessors() * 4)

@main
def main() = 
    val holders = List.fill(1_000_000)(new LazyHolder())
    val counter = new AtomicInteger()
    for (
        holder <- holders
    ) {
        ec.execute(
            () => holder.xInstant match {
                case 21 => counter.incrementAndGet()
            }
        )
    }

    new Thread() {
        override def run(): Unit = {
            while(true) {
                println(counter.get())
                Thread.sleep(1000)
            }
        }
    }.start()

@sjrd
Copy link
Member

sjrd commented Feb 2, 2023

lazy val xInstant: AnyVal

That erases to Object like AnyRef. Try an actual value class, one that extends AnyVal. Not AnyVal itself.

@szymon-rd
Copy link
Contributor

Fair point. But I tried that, and it passed as well.

@sjrd
Copy link
Member

sjrd commented Feb 2, 2023

Can you show the generated code? Perhaps we can already spot it there.

@soronpo
Copy link
Contributor Author

soronpo commented Feb 2, 2023

I pushed my (not-yet-minimized) codebase into https://github.com/DFiantHDL/DFiant under the branch lazyexception
I use AdoptOpenJDK Java 16.

Just clone, checkout lazyexception and run sbt run.
Let me know if this helps and you replicate the issue on your computer.
Note:

  • This codebase uses compiler plugins
  • This codebase was not meant to be used by "normal" people in its current state.

@szymon-rd
Copy link
Contributor

szymon-rd commented Feb 3, 2023

I pulled this repo, and it fails on this branch for me too. But are you sure that it's not the custom compiler plugin that makes it fail? Maybe you are relying on the lazy val holder (foo$lzy<n>) in it, instead of the accessor? If that were true, then this error could occur.

@soronpo
Copy link
Contributor Author

soronpo commented Feb 3, 2023

I pulled this repo, and it fails on this branch for me too. But are you sure that it's not the custom compiler plugin that makes it fail? Maybe you are relying on the lazy val holder (foo$lzy<n>) in it, instead of the accessor? If that were true, then this error could occur.

That possibility was i my mind and that is why I want find away I can minimize without the plugin or rule it out. However, if I remove the AnyVal dependency everything works correctly. Additionally, the compiler does not distinguish different runtimes, so if the fault was in the plugin, it should have tripped in a single test as well.

@szymon-rd
Copy link
Contributor

If it's a scenario where the plugin relies on direct values from the holders or accesses them in any way, it could just create a possibility of running into this race condition. Then, given right runtime scenario, it could happen. Parallel execution may affect probability of such race condition happening. As for the AnyVals, the details of execution and casting in the generated bytecode may change the execution time, and, in turn, make the probability of race condition higher. I tried analyzing this project but it's fairly big and complex, and relies on complex features like the compiler plugin (that have no compatibility guarantees). I think we would really need some kind of a minimization, because I was unfortunately unable to create any.

@soronpo
Copy link
Contributor Author

soronpo commented Feb 3, 2023

I made significant progress in minimizing it, and it indeed leads me to believe that indeed this is a compiler bug and not relevant to the plugin. It looks like I'll be able to provide a minimized example today.

@soronpo
Copy link
Contributor Author

soronpo commented Feb 3, 2023

I removed the plugin completely and the problem persists. I pushed it to the same branch.
There is still a lot more minimization to do, but at least for now we know this is indeed a compiler bug.

@soronpo
Copy link
Contributor Author

soronpo commented Feb 3, 2023

@szymon-rd I just updated the branch with the minimized version. You may need to run sbt test several times until this error takes place.

@mbovel mbovel added area:transform and removed stat:needs triage Every issue needs to have an "area" and "itype" label labels Feb 5, 2023
@szymon-rd
Copy link
Contributor

Thank you for the reproduction! I am getting right to fixing it.

@szymon-rd
Copy link
Contributor

szymon-rd commented Feb 15, 2023

Repro without sbt & munit:

//> using scala "3.3.0-RC2"

import scala.concurrent.ExecutionContext.Implicits.global
import scala.concurrent.Future
import java.util.concurrent.CountDownLatch

object Repro {


  case object DFBit 

  final class DFError extends Exception("") 
  final class DFType[+T](val value: T | DFError) extends AnyVal

  def asIR(dfType: DFType[DFBit.type]): DFBit.type = dfType.value match
    case dfTypeIR: DFBit.type => dfTypeIR
    case err: DFError         => throw new DFError

  class Holder() {
    final lazy val Bit = new DFType[DFBit.type](DFBit)
  }

  @main
  def run = 
    val cdl = new CountDownLatch(2)
    val fcdl = new CountDownLatch(2)
    var holder = new Holder()
    new Thread() {
        override def run(): Unit = 
            println("Starting 1")
            cdl.countDown()
            cdl.await()
            for (i <- 0 until 100)
                Repro.asIR(holder.Bit)
                if(i % 7 == 0) holder = new Holder()
            fcdl.countDown()
            println("Fin 1")
    }.start()

        
    new Thread() {
        override def run(): Unit = 
            println("Starting 2")
            cdl.countDown()
            cdl.await()
            for (i <- 0 until 100)
                Repro.asIR(holder.Bit)
                if(i % 9 == 0) holder = new Holder()
            fcdl.countDown()
            println("Fin 2")
    }.start()

    fcdl.await

    println("passed")
}

@soronpo
Copy link
Contributor Author

soronpo commented Feb 15, 2023

Nicely done!

@szymon-rd
Copy link
Contributor

szymon-rd commented Feb 20, 2023

Deterministic repro. I will be committing the fix soon.

//> using scala "3.3.0-RC2"
import java.util.concurrent.Semaphore

object Repro {

  case object DFBit

  final class DFError extends Exception("")
  final class DFType[+T](val value: T | DFError) extends AnyVal

  def asIR(dfType: DFType[DFBit.type]): DFBit.type = dfType.value match
    case dfTypeIR: DFBit.type => dfTypeIR
    case err: DFError         => throw new DFError


  
  object Holder {
    val s = new Semaphore(1, false)
    final lazy val Bit = {
        s.release()
        println("Entering")
        new DFType[DFBit.type](DFBit)
    }
  }

  @main
  def run = 
    new Thread() {
      override def run(): Unit = 
        Holder.s.acquire()
        println(Holder.Bit.value)
    }.start()
    new Thread() {
      override def run(): Unit = 
        Holder.s.acquire()
        println("Getting")
        println(Holder.Bit.value)

    }.start()
}

szymon-rd added a commit that referenced this issue Feb 27, 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.
@Kordyjan Kordyjan modified the milestones: 3.3.0 backports, 3.3.1 Aug 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:transform itype:bug regression This worked in a previous version but doesn't anymore stat:needs minimization Needs a self contained minimization
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants