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
@@ -337,7 +345,7 @@ object CompileTask {
       }
 
       val client = state.client
-      CompileGraph
+      val compilation = CompileGraph
         .traverse(dag, client, store, bestEffortAllowed, setup, compile, tracer)
         .flatMap { pdag =>
           val partialResults = Dag.dfs(pdag, mode = Dag.PreOrder)
@@ -408,7 +416,12 @@ object CompileTask {
               .doOnFinish(_ => Task(rootTracer.terminate()))
           }
         }
+      compilation
+        .doOnFinish(_ =>
+          cleanupIfNoCompilationRunning(compilationCounter.decrementAndGet(), tracer, rawLogger)
+        )
     }
+
   }
 
   case class ConfiguredCompilation(scalacOptions: List[String])
@@ -470,13 +483,14 @@ object CompileTask {
             for {
               _ <- previousSuccessful.populatingProducts
               _ <- populateNewProductsTask
-              _ <- cleanUpPreviousResult(
+              toDelete = previousDirectoriesToCleanup(
                 previousSuccessful,
                 previousResult,
                 compilerResult,
                 tracer,
                 logger
               )
+              _ = toDelete.foreach(cleanUpTasks.add)
             } yield ()
         }
 
@@ -514,15 +528,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 +545,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 +574,34 @@ 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) =>
+    List(
+      previousReadOnlyToDelete,
+      previousBestEffortToDelete
+    ).flatten
+
+  }
+
+  def cleanupIfNoCompilationRunning(
+      counter: Int,
+      tracer: BraveTracer,
+      logger: Logger
+  ): Task[Unit] = {
+    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")
+              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(_ => ())
   }
 }

frontend/src/main/scala/bloop/engine/tasks/compilation/CompileGatekeeper.scala

@@ -1,18 +1,22 @@
 package bloop.engine.tasks.compilation
 
-import bloop.{Compiler, UniqueCompileInputs}
-import bloop.data.{ClientInfo, Project}
+import java.util.concurrent.ConcurrentHashMap
+
+import bloop.Compiler
+import bloop.UniqueCompileInputs
+import bloop.data.ClientInfo
+import bloop.data.Project
 import bloop.engine.Dag
 import bloop.engine.caches.LastSuccessfulResult
-import bloop.io.AbsolutePath
-import bloop.logging.{DebugFilter, Logger, LoggerAction}
+import bloop.logging.DebugFilter
+import bloop.logging.Logger
+import bloop.logging.LoggerAction
 import bloop.reporter.ReporterAction
 import bloop.task.Task
 import bloop.tracing.BraveTracer
-import monix.execution.atomic.{AtomicBoolean, AtomicInt}
-import monix.reactive.Observable
 
-import java.util.concurrent.ConcurrentHashMap
+import monix.execution.atomic.AtomicBoolean
+import monix.reactive.Observable
 
 object CompileGatekeeper {
   private implicit val filter: DebugFilter = DebugFilter.Compilation
@@ -28,7 +32,6 @@ object CompileGatekeeper {
 
   /* -------------------------------------------------------------------------------------------- */
 
-  private val currentlyUsedClassesDirs = new ConcurrentHashMap[AbsolutePath, AtomicInt]()
   private val runningCompilations = new ConcurrentHashMap[UniqueCompileInputs, RunningCompilation]()
   private val lastSuccessfulResults = new ConcurrentHashMap[ProjectId, LastSuccessfulResult]()
 
@@ -62,39 +65,6 @@ object CompileGatekeeper {
               "Found matching compilation",
               ("uniqueInputs", bundle.uniqueInputs.toString)
             ) { tracer =>
-              val usedClassesDir = running.usedLastSuccessful.classesDir
-              val usedClassesDirCounter = running.usedLastSuccessful.counterForClassesDir
-
-              usedClassesDirCounter.getAndTransform { count =>
-                if (count == 0) {
-                  tracer.trace(
-                    "Aborting deduplication",
-                    ("uniqueInputs", bundle.uniqueInputs.toString)
-                  ) { tracer =>
-                    logger.debug(
-                      s"Abort deduplication, dir is scheduled to be deleted in background:${bundle.uniqueInputs}"
-                    )
-                    // Abort deduplication, dir is scheduled to be deleted in background
-                    deduplicate = false
-                    // Remove from map of used classes dirs in case it hasn't already been
-                    currentlyUsedClassesDirs.remove(usedClassesDir, usedClassesDirCounter)
-                    // Return previous count, this counter will soon be deallocated
-                    count
-                  }
-                } else {
-                  tracer.trace(
-                    "Increasing compilation counter",
-                    ("uniqueInputs", bundle.uniqueInputs.toString)
-                  ) { tracer =>
-                    logger.debug(
-                      s"Increase count to prevent other compiles to schedule its deletion:${bundle.uniqueInputs}"
-                    )
-                    // Increase count to prevent other compiles to schedule its deletion
-                    count + 1
-                  }
-                }
-              }
-
               if (deduplicate) running
               else scheduleCompilation(inputs, bundle, client, compile, tracer)
             }
@@ -139,8 +109,6 @@ object CompileGatekeeper {
       import bundle.logger
       import inputs.project
 
-      var counterForUsedClassesDir: AtomicInt = null
-
       def initializeLastSuccessful(previousOrNull: LastSuccessfulResult): LastSuccessfulResult =
         tracer.trace(s"initialize last successful") { _ =>
           val result = Option(previousOrNull).getOrElse(bundle.lastSuccessful)
@@ -156,7 +124,6 @@ object CompileGatekeeper {
               // Replace classes dir, counter and populating with values from previous for correctness
               .copy(
                 classesDir = result.classesDir,
-                counterForClassesDir = result.counterForClassesDir,
                 populatingProducts = result.populatingProducts
               )
           } else {
@@ -176,32 +143,7 @@ object CompileGatekeeper {
                 s"Return previous result or the initial last successful coming from the bundle:${project.uniqueId}"
               )
               // Return previous result or the initial last successful coming from the bundle
-              val previousResult = initializeLastSuccessful(previousResultOrNull)
-
-              currentlyUsedClassesDirs.compute(
-                previousResult.classesDir,
-                (_: AbsolutePath, counter: AtomicInt) => {
-                  logger.debug(
-                    s"Set counter for used classes dir when init or incrementing:${previousResult.classesDir}"
-                  )
-                  // Set counter for used classes dir when init or incrementing
-                  if (counter == null) {
-                    logger.debug(s"Create new counter:${previousResult.classesDir}")
-                    val initialCounter = AtomicInt(1)
-                    counterForUsedClassesDir = initialCounter
-                    initialCounter
-                  } else {
-                    counterForUsedClassesDir = counter
-                    val newCount = counter.incrementAndGet(1)
-                    logger.debug(
-                      s"Increasing counter for ${previousResult.classesDir} to $newCount"
-                    )
-                    counter
-                  }
-                }
-              )
-
-              previousResult.copy(counterForClassesDir = counterForUsedClassesDir)
+              initializeLastSuccessful(previousResultOrNull)
             }
           )
         }