Reimplement PipelineMain's sequencing to avoid races

[dwijnand]

We've hit this a number of times:

• Precompile -i files for script runner #8349 (comment)
• Fix warnings in doc comments #8355 (comment)
• Mention -opt:help & opt-warnings:help #8683 (comment)
• and again in There is no more -Xsource:2.14, only -Xsource:3 #8812

And some other misc fixes & cleanups.

[retronym]

I think the underlying problem is elsewhere. Maybe PipelineMain are ignoring an compile error in an trying to compiler a downstream project anyway? Or, similarly, ignoring an error in outline compilation and trying to do the full compilation anyway.

Reviewing the code, I think the root cause might be that def outlineCompile checks this.reporter.hasErrors rather than compiler.reporter.hasErrors. I saw a few other suspect looking things in PipelineMain and ended up with this diff:

diff --git a/src/compiler/scala/tools/nsc/PipelineMain.scala b/src/compiler/scala/tools/nsc/PipelineMain.scala
index ee97797447..e83a756032 100644
--- a/src/compiler/scala/tools/nsc/PipelineMain.scala
+++ b/src/compiler/scala/tools/nsc/PipelineMain.scala
@@ -154,9 +154,11 @@ class PipelineMainClass(argFiles: Seq[Path], pipelineSettings: PipelineMain.Pipe
       val counter = new AtomicInteger(count)
       val failed = new AtomicBoolean(false)
       val handler = (a: Try[_]) => a match {
-        case f @ Failure(_) =>
+        case f @ Failure(t) =>
           if (failed.compareAndSet(false, true)) {
-            done.complete(f)
+            val allFailures: immutable.Seq[Throwable] = allFutures.flatMap(_.value).flatMap(_.failed.toOption)
+            allFailures.foreach(_.printStackTrace())
+            done.complete(Failure(t))
           }
         case Success(_) =>
           val remaining = counter.decrementAndGet()
@@ -431,6 +433,7 @@ class PipelineMainClass(argFiles: Seq[Path], pipelineSettings: PipelineMain.Pipe
         reporter.flush()
       else if (command.shouldStopWithInfo)
         reporter.echo(command.getInfoMessage(result))
+      result.reporter = createReporter(result.settings)
       result
     } catch {
       case t: Throwable =>
@@ -450,8 +453,8 @@ class PipelineMainClass(argFiles: Seq[Path], pipelineSettings: PipelineMain.Pipe
         run1 compile files
         outlineTimer.stop()
         log(f"scalac outline: done ${outlineTimer.durationMs}%.0f ms")
-        reporter.finish()
-        if (reporter.hasErrors) {
+        compiler.reporter.finish()
+        if (compiler.reporter.hasErrors) {
           log("scalac outline: failed")
           outlineDone.complete(Failure(new RuntimeException(label + ": compile failed: ")))
         } else {
@@ -460,8 +463,7 @@ class PipelineMainClass(argFiles: Seq[Path], pipelineSettings: PipelineMain.Pipe
         }
       } catch {
         case t: Throwable =>
-          t.printStackTrace()
-          outlineDone.complete(Failure(new RuntimeException(label + ": compile failed: ")))
+          outlineDone.complete(Failure(new RuntimeException(label + ": compile failed: ", t)))
       }
     }
 
@@ -488,9 +490,7 @@ class PipelineMainClass(argFiles: Seq[Path], pipelineSettings: PipelineMain.Pipe
                 log(f"scalac (${ix + 1}/$groupCount): done ${group.timer.durationMs}%.0f ms")
               }
               if (compiler2.reporter.hasErrors) {
-                group.done.complete(Failure(new RuntimeException(label + ": compile failed: ")))
-              } else {
-                group.done.complete(Success(()))
+                throw new RuntimeException(label + ": compile failed: ")
               }
             } finally {
               compiler2.close()
@@ -575,7 +575,7 @@ class PipelineMainClass(argFiles: Seq[Path], pipelineSettings: PipelineMain.Pipe
             log(f"javac: done ${javaTimer.durationMs}%.0f ms ")
           } else {
             javaTimer.stop()
-            log(f"javac: error ${javaTimer.durationMs}%.0f ms ")
+            throw new RuntimeException(f"javac: error ${javaTimer.durationMs}%.0f ms ")
           }
           ()
         })

[dwijnand]

I think the underlying problem is elsewhere. Maybe PipelineMain are ignoring an compile error in an trying to compiler a downstream project anyway? Or, similarly, ignoring an error in outline compilation and trying to do the full compilation anyway.

Why do you think it's elsewhere? It's a thrown NoSuchFileException after Files.exist check, which shouts "race condition" to me.

But I'll throw your diff in too, then.

[eed3si9n]

scala/src/compiler/scala/tools/nsc/PipelineMain.scala

Lines 152 to 170 in 2d9357e

	def awaitAll(fs: Seq[Future[_]]): Future[_] = {
	val done = Promise[Any]()
	val allFutures = projects.flatMap(_.futures)
	val count = allFutures.size
	val counter = new AtomicInteger(count)
	val failed = new AtomicBoolean(false)
	val handler = (a: Try[_]) => a match {
	case f @ Failure(_) =>
	if (failed.compareAndSet(false, true)) {
	done.complete(f)
	}
	case Success(_) =>
	val remaining = counter.decrementAndGet()
	if (remaining == 0) done.success(())
	}
	allFutures.foreach(_.onComplete(handler))
	done.future
	}

In the above, awaitAll completes done in case any of the allFutures fail, which means if you have two subprojects one that fails and one that succeeds, the composed future would complete before the successful one is done. Scripted test in my Zinc PR uses Future.sequence, but I ran into similar issue, and in my case it I needed to add a Future that waits for all regular JARs - sbt/zinc@e54c4dd

[dwijnand]

which means if you have two subprojects one that fails and one that succeeds, the composed future would complete the successful one is done.

You're missing a word there somewhere, so I don't understand what you're saying. Isn't the counter setting things up right?

[eed3si9n]

@dwijnand it's missing "before" -- the composed future would complete before the successful one is done. In case of a failure in one subproject, done.complete(f) gets called while some compilations might be still going on.

[dwijnand]

Right. What's wrong with that?

[eed3si9n]

When the test exits I am guessing that /tmp/scala.picklecache7643988611803649817/tmp/pipelineBase3274412707466812224/ would get deleted and you're seeing java.nio.file.NoSuchFileException: /tmp/scala.picklecache7643988611803649817/tmp/pipelineBase3274412707466812224/projects/m1/p1/target.jar when the successful subproject completes its compilation.

[retronym]

Maybe the correct building block we're after is:

    def sequenceFailSlow[A](fs: Seq[Future[A]]): Future[Seq[A]] = {
      val futures: Seq[Future[Either[Throwable, A]]] = fs.map(_.transform(tr => Success(tr.toEither)))
      Future.sequence(futures).map {
        results =>
          val successes = results.collect { case Right(value) => value }
          val failures = results.collect { case Left(throwable) => throwable }.toList
          failures match {
            case head :: rest => rest.foreach(head.addSuppressed(_)); throw head
            case Nil =>
              successes
          }
      }
    }

    def awaitDone(): Unit = {
      val allFutures: immutable.Seq[Future[_]] = projects.flatMap(_.futures)
      val numAllFutures = allFutures.size
      val awaitAllFutures: Future[_] = sequenceFailSlow[Any](allFutures)

[retronym]

It's a thrown NoSuchFileException after Files.exist check, which shouts "race condition" to me.

Echoing @eed3si9n's analysis: yes, this indicates a race. But if we remove the symptom of the NoSuchFileException, the test will fail anyway when the downstream project is compiled with the file absent from its classpath.