bugfix: Only delete directories if nothing is running

Previously, we would sometimes delete directories when another compilation was running concurrently. Now, we only delete at the end if no other compilation is running.

Author: tgodzik

backend/src/main/scala/bloop/Compiler.scala

@@ -435,7 +435,6 @@ object Compiler {
               // new compilation artifacts coming from scalac, which we will not
               // have in this case and it's going to remain empty.
               val firstTask = Task { BloopPaths.delete(AbsolutePath(newClassesDir)) }
-
               // Then we copy previous best effort artifacts to a clientDir from the
               // cached compilation result.
               // This is useful if e.g. the client restarted after the last compilation

frontend/src/main/scala/bloop/engine/caches/LastSuccessfulResult.scala

@@ -9,7 +9,6 @@ import bloop.data.Project
 import bloop.io.AbsolutePath
 import bloop.task.Task
 
-import monix.execution.atomic.AtomicInt
 import xsbti.compile.CompileAnalysis
 import xsbti.compile.MiniSetup
 import xsbti.compile.PreviousResult
@@ -18,7 +17,6 @@ case class LastSuccessfulResult(
     noClassPathAndSources: Boolean,
     previous: PreviousResult,
     classesDir: AbsolutePath,
-    counterForClassesDir: AtomicInt,
     populatingProducts: Task[Unit]
 ) {
   def isEmpty: Boolean = {
@@ -43,7 +41,6 @@ object LastSuccessfulResult {
       noClassPathAndSources = true,
       EmptyPreviousResult,
       emptyClassesDir,
-      AtomicInt(0),
       Task.now(())
     )
   }
@@ -57,7 +54,6 @@ object LastSuccessfulResult {
       noClassPathAndSources = inputs.sources.size == 0 && inputs.classpath.size == 0,
       products.resultForFutureCompilationRuns,
       AbsolutePath(products.newClassesDir),
-      AtomicInt(0),
       backgroundIO
     )
   }

frontend/src/main/scala/bloop/engine/tasks/CompileTask.scala

@@ -1,8 +1,13 @@
 package bloop.engine.tasks
 
+import java.nio.file.Path
 import java.util.Optional
+import java.util.concurrent.ConcurrentLinkedQueue
+import java.util.concurrent.atomic.AtomicInteger
+
 import scala.collection.mutable
 import scala.concurrent.Promise
+
 import bloop.CompileBackgroundTasks
 import bloop.CompileInputs
 import bloop.CompileOutPaths
@@ -35,17 +40,18 @@ import bloop.reporter.ReporterInputs
 import bloop.task.Task
 import bloop.tracing.BraveTracer
 import bloop.util.BestEffortUtils.BestEffortProducts
+
 import monix.execution.CancelableFuture
 import monix.reactive.MulticastStrategy
 import monix.reactive.Observable
 import xsbti.compile.CompileAnalysis
 import xsbti.compile.MiniSetup
 import xsbti.compile.PreviousResult
 
-import java.nio.file.Path
-
 object CompileTask {
   private implicit val logContext: DebugFilter = DebugFilter.Compilation
+  private val compilationCounter = new AtomicInteger(0)
+  private val cleanUpTasks = new ConcurrentLinkedQueue[AbsolutePath]()
 
   def compile[UseSiteLogger <: Logger](
       state: State,
@@ -57,6 +63,8 @@ object CompileTask {
       store: CompileClientStore,
       rawLogger: UseSiteLogger
   ): Task[State] = Task.defer {
+    compilationCounter.incrementAndGet()
+
     import bloop.data.ClientInfo
     import bloop.internal.build.BuildInfo
     val originUri = state.build.origin
@@ -335,16 +343,15 @@ object CompileTask {
           o
         )
       }
-
       val client = state.client
       CompileGraph
         .traverse(dag, client, store, bestEffortAllowed, setup, compile, tracer)
         .flatMap { pdag =>
           val partialResults = Dag.dfs(pdag, mode = Dag.PreOrder)
           val finalResults = partialResults.map(r => PartialCompileResult.toFinalResult(r))
           Task.gatherUnordered(finalResults).map(_.flatten).flatMap { results =>
-            val cleanUpTasksToRunInBackground =
-              markUnusedClassesDirAndCollectCleanUpTasks(results, state, tracer, rawLogger)
+            val makrUnusedDirectoriesTask =
+              markUnusedClassesDir(results, state, tracer, rawLogger)
 
             val failures = results.flatMap {
               case FinalNormalCompileResult(p, results) =>
@@ -389,7 +396,14 @@ object CompileTask {
             }
 
             // Schedule to run clean-up tasks in the background
-            runIOTasksInParallel(cleanUpTasksToRunInBackground)
+            runIOTasksInParallel(
+              makrUnusedDirectoriesTask,
+              runOnFinish = cleanupIfNoCompilationRunningTask(
+                compilationCounter.decrementAndGet(),
+                tracer,
+                rawLogger
+              )
+            )
 
             val runningTasksRequiredForCorrectness = Task.sequence {
               results.flatMap {
@@ -409,6 +423,7 @@ object CompileTask {
           }
         }
     }
+
   }
 
   case class ConfiguredCompilation(scalacOptions: List[String])
@@ -445,14 +460,14 @@ object CompileTask {
     }
   }
 
-  private def markUnusedClassesDirAndCollectCleanUpTasks(
+  private def markUnusedClassesDir(
       results: List[FinalCompileResult],
       previousState: State,
       tracer: BraveTracer,
       logger: Logger
   ): List[Task[Unit]] =
     tracer.trace("markUnusedClassesDirAndCollectCleanUpTasks") { _ =>
-      val cleanUpTasksToSpawnInBackground = mutable.ListBuffer[Task[Unit]]()
+      val markDeleteTasks = mutable.ListBuffer[Task[Unit]]()
       results.foreach { finalResult =>
         val resultBundle = finalResult.result
         val newSuccessful = resultBundle.successful
@@ -464,36 +479,40 @@ object CompileTask {
             case _ => None
           }
         val populateNewProductsTask = newSuccessful.map(_.populatingProducts).getOrElse(Task.unit)
-        val cleanUpPreviousLastSuccessful = resultBundle.previous match {
+        val markDeletePreviousLastSuccessful = resultBundle.previous match {
           case None => populateNewProductsTask
           case Some(previousSuccessful) =>
             for {
               _ <- previousSuccessful.populatingProducts
               _ <- populateNewProductsTask
-              _ <- cleanUpPreviousResult(
+              toDelete = previousDirectoriesToCleanup(
                 previousSuccessful,
                 previousResult,
                 compilerResult,
                 tracer,
                 logger
               )
+              _ = toDelete.foreach(cleanUpTasks.add)
             } yield ()
         }
 
-        cleanUpTasksToSpawnInBackground.+=(cleanUpPreviousLastSuccessful)
+        markDeleteTasks.+=(markDeletePreviousLastSuccessful)
       }
 
-      cleanUpTasksToSpawnInBackground.toList
+      markDeleteTasks.toList
     }
 
   def runIOTasksInParallel[T](
       tasks: Traversable[Task[T]],
-      parallelUnits: Int = Runtime.getRuntime().availableProcessors()
+      parallelUnits: Int = Runtime.getRuntime().availableProcessors(),
+      runOnFinish: Task[Unit]
   ): Unit = {
     val aggregatedTask = Task.sequence(
       tasks.toList.grouped(parallelUnits).map(group => Task.gatherUnordered(group)).toList
     )
-    aggregatedTask.map(_ => ()).runAsync(ExecutionContext.ioScheduler)
+    aggregatedTask
+      .flatMap(_ => runOnFinish)
+      .runAsync(ExecutionContext.ioScheduler)
     ()
   }
 
@@ -514,15 +533,14 @@ object CompileTask {
    * superseeded by the new classes directory generated during a successful
    * compile.
    */
-  private def cleanUpPreviousResult(
+  private def previousDirectoriesToCleanup(
       previousSuccessful: LastSuccessfulResult,
       previousResult: Option[Compiler.Result],
       compilerResult: Compiler.Result,
       tracer: BraveTracer,
       logger: Logger
-  ): Task[Unit] = tracer.trace("clean up previous result") { implicit tracer =>
+  ): List[AbsolutePath] = tracer.trace("clean up previous result") { _ =>
     val previousClassesDir = previousSuccessful.classesDir
-    val currentlyUsedCounter = previousSuccessful.counterForClassesDir.decrementAndGet(1)
 
     val previousReadOnlyToDelete = compilerResult match {
       case Success(_, products, _, _, isNoOp, _, _) =>
@@ -532,11 +550,6 @@ object CompileTask {
         } else if (CompileOutPaths.hasEmptyClassesDir(previousClassesDir)) {
           logger.debug(s"Skipping delete of empty classes dir ${previousClassesDir}")
           None
-        } else if (currentlyUsedCounter != 0) {
-          logger.debug(
-            s"Skipping delete of $previousClassesDir, counter is $currentlyUsedCounter"
-          )
-          None
         } else {
           val newClassesDir = products.newClassesDir
           logger.debug(s"Scheduling to delete ${previousClassesDir} superseded by $newClassesDir")
@@ -566,28 +579,36 @@ object CompileTask {
       case _ => None
     }
 
-    def deleteOrphanDir(orphanDir: Option[AbsolutePath])(implicit
-        tracer: BraveTracer
-    ): Task[Unit] = {
-      tracer.trace("delete orphan dir") { _ =>
-        orphanDir match {
-          case None => Task.unit
-          case Some(classesDir) =>
-            Task.eval {
-              logger.debug(s"Deleting contents of orphan dir $classesDir")
-              BloopPaths.delete(classesDir)
-            }.asyncBoundary
+    List(
+      previousReadOnlyToDelete,
+      previousBestEffortToDelete
+    ).flatten
+
+  }
+
+  def cleanupIfNoCompilationRunningTask(
+      counter: Int,
+      tracer: BraveTracer,
+      logger: Logger
+  ): Task[Unit] = {
+    Task {
+      if (counter == 0) {
+        tracer.trace("delete orphan directories") { _ =>
+          val allDeleteDirectories = new mutable.ListBuffer[AbsolutePath]()
+          while (cleanUpTasks.size() > 0) allDeleteDirectories += cleanUpTasks.poll()
+          val allDeleteTasks = allDeleteDirectories
+            .result()
+            .map(classesDir => {
+              Task.eval {
+                logger.debug(s"Deleting contents of orphan dir $classesDir : ${classesDir.exists}")
+                BloopPaths.delete(classesDir)
+              }.asyncBoundary
+            })
+          Task.gatherUnordered(allDeleteTasks).map(_ => ())
         }
+      } else {
+        Task.unit
       }
-    }
-
-    Task
-      .gatherUnordered(
-        List(
-          deleteOrphanDir(previousReadOnlyToDelete),
-          deleteOrphanDir(previousBestEffortToDelete)
-        )
-      )
-      .map(_ => ())
+    }.flatten
   }
 }