scala / scala3

The Scala 3 compiler, also known as Dotty.
https://dotty.epfl.ch
Apache License 2.0
5.8k stars 1.04k forks source link

Debugging experience of lazy val fields (simple to cause deadlock) #21298

Closed vasilmkd closed 12 hours ago

vasilmkd commented 1 month ago

Important: technically this is not a bug, but it leads to unrecoverable deadlocks during debugging

Compiler version

All Scala 3 versions.

Affects both Metals and IntelliJ IDEA's debuggers.

Minimized example

Let's use Scala 3.4.2 as the latest stable release, but really, any version is the same.

// Foo.scala
class Foo:
  lazy val bar1: String =
    println("hello 1")
    println("hello 2") // put a breakpoint here (important that it isn't on the first line of the lazy val, which is an ambiguous breakpoint)
    "abc"

  lazy val bar2: String =
    println("hello 3")
    println("hello 4")
    bar1
end Foo
// main.scala
@main
def main(): Unit =
  val foo = Foo()
  println(foo.bar1)

Debug the program. The debugger should stop on println("hello 2") inside the initialization method of the bar1 lazy val. Now, evaluate bar2.

Result: "hello 3" and "hello 4" will be printed in the debugger console, but evaluation will not finish. In fact, this is a deadlock. Trying to resume the program (Continue) does nothing. The only thing that can be done is to press Stop to terminate the debugged program. For the VS Code debugger specifically, it will leave the debugged program in a hanging state. The program (main) can be seen as running in the terminal with jps.

And all this time, the lazy val encoding works completely as intended, originally described in SIP-20.

Here's exactly how the debugger gets deadlocked.

The JVM debuggers in VS Code and in IntelliJ IDEA use one main debugger thread for controlling the debugged VM. When we arrive at the println("hello 2") breakpoint, the debugged VM is suspended and awaiting further instructions from the debugger.

The VM is suspended in the initialization method of the bar1 lazy val. At this point, bar1 is in the scala.runtime.LazyVals.Evaluating state (or state 1 in the "old" lazy val encoding used in Scala 3.0, 3.1 and 3.2).

From the Scaladoc of object Evaluating:

  /**
   * Used to indicate the state of a lazy val that is currently being
   * evaluated with no other thread awaiting its result.
   */
  object Evaluating extends LazyValControlState

We then try to evaluate bar2 in the debugger. This calls the bar2() method on class Foo, which follows the laws of the lazy val initialization protocol and either initializes the value, waits until it becomes initialized, or returns the already computed value.

The initialization of bar2 begins, "hello 3" and "hello 4" are printed. Then, bar1 is hit again. We already know that bar1 is already being initialized by someone else. According to the lazy val protocol, we now have to wait for its value to become available before continuing. So, the thread that evaluates bar2 has to suspend while bar1 is being evaluated. But the thread that evaluates bar2 is the main debugger thread. So now, the debugger is essentially taken out of action. Pressing Continue, or Step Into, or Step Over, or any other command except Stop, is ignored from the perspective of the user. These commands are not ignored, they are simply queued behind the current action being run on the debugger thread and will be processed as soon as this one finishes. Except, the current action cannot finish unless someone resumes the debugged program, and no one can do that, so, deadlock.

Debugger experience

First of all, I'm opening this ticket in order to raise awareness of this issue.

We can debate what, if anything, can be done or needs to be done about this. My intention is to simply present a case where a fundamental Scala language feature can easily be misused by users.

Regarding the example with lazy vals referencing each other. I don't think that this is an edge case, or isn't a representative example. There exists software where high-initialization cost resources are initialized once and cached, and this is represented using lazy val, and multiple lazy vals in a single class that do reference each other (or also across classes).

Furthermore, I developed what I thought was a cool feature in the IDEA debugger, the ability to manually initialize an uninitialized lazy val. We've decided to remove the Initialize button in upcoming releases, exactly due to the problems outlined in this ticket. The Initialize button is not evil in any way, but it does encourage users to click it simply by virtue of being interactive, which may or may not lead to a deadlock of the debugger. Of course, the issue with manually evaluating expressions leading to a deadlock still exists.

Screenshot 2024-07-30 at 13 44 32

This is a feature that Kotlin supports, for example, in IDEA. I currently do not know how exactly how by lazy delegates are implemented in Kotlin and whether they suffer from any concurrency issues from the language perspective (like Scala 2 did), but nonetheless, they are confident enough to offer this feature in the debugger.

I would be interested in members of the Scala team sharing their thoughts on this. I'm tagging @adpi2 as the main person working on the debugging experience, but others are of course more than welcome to chime in.

Thanks in advance.

Gedochao commented 1 month ago

cc @adpi2 @tgodzik @kasiaMarek @rochala

tgodzik commented 1 month ago

Could we not just run evaluation in a separate thread?

Gedochao commented 1 month ago

@vasilmkd note: perhaps we should track this in DAP's issue tracker, rather than here? (https://github.com/scalacenter/scala-debug-adapter/issues)

vasilmkd commented 1 month ago

Could we not just run evaluation in a separate thread?

This would have to be supported by the Microsoft JVM Debugger. Then, IDEA would have to support it in the JVM debugger as well, which I haven't yet received info whether this is possible.

note: perhaps we should track this in DAP's issue tracker, rather than here? (https://github.com/scalacenter/scala-debug-adapter/issues)

I don't mind.

adpi2 commented 1 month ago

I guess you don't have the same issue in Scala 2 where the encoding of lazy val is different.

Considering this piece of code:

class Foo {
  lazy val bar1: String = {
    println("init bar1")
    bar2
  }

  lazy val bar2: String = {
    println("init bar2")
    bar1
  }
}

val foo = new Foo
foo.bar1

In Scala 3 it produces:

init bar1
init bar2
// deadlock

Whereas in Scala 2 it produces:

init bar1
init bar2
init bar1
init bar2
init bar1
...
java.lang.StackOverflowError

In a sense the Scala 2 behavior is better because it reflects the infinite recursive call between bar1 and bar2. But the Scala 2 encoding is also a lot more expensive than the Scala 3 one. It is based on synchronized in Scala 2, whereas it uses CAS in Scala 3.

So it seems the current behavior of the debugger is correct: if we evaluate something that should deadlock, then it should deadlock the evaluating thread.

Now the questions are:

  1. Can we avoid the deadlock of recursive lazy val initializations? (Independently of the debugger)

    Probably not: the Scala 3 initialization check is much faster and we probably don't want to backtrack on it.

  2. Can we catch a deadlock from the debugger and interrupt the thread?

Yes maybe we can have some sort of timeout or a button to interrupt a waiting thread, but I am afraid it would leave bar2 in a corrupted state and we won't be able to initialize it anymore.

  1. Should we evaluate in a different thread?

If we do so, then we would get a deadlock on the evaluation of Scala 2 lazy vals because of the synchronized initializations: only one thread can initialize Scala 2 lazy vals of the same object at a time (reentrancy).

In general, if we pause the debugger in a synchronized block, I think we should be able to evaluate other synchronized blocks of the same object.

  1. How to prevent users from evaluating deadlocking expressions?

Probably the IntelliJ debugger should not encourage its users to fall into deadlocks. One idea I can think of is to disable the Initialization feature as soon as there is an initialization of a lazy val in the current call stack.

vasilmkd commented 4 days ago

Responding to this ticket slipped through the cracks, I'm sorry for the delay.

  1. Completely agree.
  2. Completely agree.
  3. In general, if we pause the debugger in a synchronized block, I think we should be able to evaluate other synchronized blocks of the same object.

    Yes, this is possible.

  4. The initialization feature is currently disabled. I agree that the debugger should not encourage users to fall into deadlocks.
adpi2 commented 12 hours ago

So I think we agree there is nothing we can do on the compiler side to improve this.