From 795c1ae818a460da3b2ff338eecabd144950fe8e Mon Sep 17 00:00:00 2001 From: Guillaume Martres Date: Mon, 22 May 2017 20:49:51 +0200 Subject: [PATCH] Fix race conditions due to non-volatile lazy vals These races could be observed by running: dotty-compiler-bootstrapped/test-only *.CompilationTests These never happened on the CI so far because the order in which the classes are run is not deterministic and we got "lucky" that the first tests running the compiler did not use parallelism. The races were caused by a combination of multiple factors: - The new classpath implementation imported from 2.12 has a shared global cache of ClassPath instances (see ZipAndJarFileLookupFactory.scala) - If multiple threads try to initialize a lazy val in dotty, only one will succeed and the other will get null (in scalac, each thread will redo the initialization) - The classpath implementation relies on code from scala.reflect.io that was recently imported into dotty, this code contains fields with lazy vals. The race with the lazy val in ZipArchive was easy to fix since it immediately lead to NullPointerException, but the one in AbstractFile was much harder to find because the only method called on "lazy val extension" is `==` which works with null. This suggests that we should implement a compiler option to check for lazy val races at runtime. In both cases we fixed the issue by simply dropping the `lazy` instead of adding an `@volatile`, for ZipArchive I cannot think of a usecase where you would not want to force the lazy val, and for `AbstractFile` the cost of always computing the extension is probably less than adding a lock. --- compiler/src/dotty/tools/io/AbstractFile.scala | 2 +- compiler/src/dotty/tools/io/ZipArchive.scala | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/compiler/src/dotty/tools/io/AbstractFile.scala b/compiler/src/dotty/tools/io/AbstractFile.scala index 1ae9bcea423b..676113dfdb5b 100644 --- a/compiler/src/dotty/tools/io/AbstractFile.scala +++ b/compiler/src/dotty/tools/io/AbstractFile.scala @@ -98,7 +98,7 @@ abstract class AbstractFile extends Iterable[AbstractFile] { /** Checks extension case insensitively. */ def hasExtension(other: String) = extension == other.toLowerCase - private lazy val extension: String = Path.extension(name) + private val extension: String = Path.extension(name) /** The absolute file, if this is a relative file. */ def absolute: AbstractFile diff --git a/compiler/src/dotty/tools/io/ZipArchive.scala b/compiler/src/dotty/tools/io/ZipArchive.scala index 6b7c838b638d..9a339a73b0e4 100644 --- a/compiler/src/dotty/tools/io/ZipArchive.scala +++ b/compiler/src/dotty/tools/io/ZipArchive.scala @@ -149,7 +149,7 @@ final class FileZipArchive(file: JFile) extends ZipArchive(file) { override def sizeOption: Option[Int] = Some(zipEntry.getSize.toInt) } - lazy val (root, allDirs) = { + val (root, allDirs) = { val root = new DirEntry("/") val dirs = mutable.HashMap[String, DirEntry]("/" -> root) val zipFile = openZipFile()