Dotty: lazy val locking

[baszo]

I wrote a comment on SIP-20. And I think problem is also with dotty because it is based on same concept.

I found a problem with implementation of lazy val. When one thread will initialize lazy val and other check that val is already initializing then call wait4Notification and set flag to 2. Then it is possibility that first thread finished initializing and checked that flag which is set to 2 and do notifyAll on monitor. After that second thread will call wait and it will lock forever. Problem is with time between setting flag and wait on monitor.

My solution is :

@inline def wait4Notification(t: Object, offset: Long, cur: Long, ord: Int) = {
  println(s"${Thread.currentThread().getId()}-wait4Notification($t, $offset, $cur, $ord)")
  var retry = true
  while (retry) {
    val cur = get(t, offset)
    val state = STATE(cur, ord)
    if (state == 1) {
      val monitor = getMonitor(t, ord)
      //make sure that thread will wait on monitor
      monitor.synchronized {
        if(CAS(t, offset, cur, 2, ord))
          monitor.wait()
      }
    }
    else if (state == 2) {
      val monitor = getMonitor(t, ord)
      monitor.synchronized {
        println(s"${Thread.currentThread().getId} -$monitor - wait")
        monitor.wait()
      }
    }
    else retry = false
  }
}

[DarkDimius]

Hi Andrzej,

Thanks for writing. You are right that the implementation has a formal bug. Strange that we haven't seen it ever occur in hours and hours of benchmarking the implementation both done by me and Alex.

Unfortunately your fix is both inefficient and incorrect:

• inefficient in making CAS inside the first synchchronized block. it's a very pessimistic act that is rarely needed;
• incorrect in that the second synchronized block still has the same issue and can lead to a thread waiting forever.

Could you please come to https://github.com/lampepfl/dotty/pull/2276 and review my proposed fix?

Thank you,
Dmitry

[DarkDimius]

// I've modified your comment to fix github formatting.

[nicolasstucki]

What is the status of this fix?

[Blaisorblade]

The issue is still there, see #2276 (and #2535).

[smarter]

I think I just hit this for the first time, I was running our test suite but it got stuck at the very first test in dotty.tools.dotc.FromTastyTests.runTestFromTasty, see https://gist.github.com/smarter/588b5dcb19f26f7069c1f2b393995a56 for the jstack output.

[smarter]

Just hit this again in dotty.tools.io.FileZipArchive.$1$ while compiling https://github.com/PDBP/pdbp. I'm raising the priority of this because this is really a critical bug we need to fix somehow.

[allanrenucci]

It seems I can reliably reproduce the issue with the following program:

object Test {
  var count = 0

  @volatile lazy val x: Int = {
    if (count < 100) {
      count += 1
      ???
    }
    1
  }

  def main(args: Array[String]): Unit = {
    def fetchLazy(): Unit = try x catch { case _: Throwable => fetchLazy() }

    for (_ <- 0 until 10) {
      new Thread(() => fetchLazy()).start()
    }
  }
}

[allanrenucci]

Since we have concerns about the size of the generated code and the correctness of the new implementation, I propose we revert to the Scala 2 implementation for thread safe lazy fields. One can always revive SIP-20 in the future.