refactor: Fix possible concurrency issues after review

build.sbt

@@ -203,7 +203,7 @@ val sharedScalacOptions = List(
   scalacOptions ++= {
     CrossVersion.partialVersion(scalaVersion.value) match {
       case partialVersion
-          // Scala 2.12.16 aand lower break within the tests if target is set to 11
+          // Scala 2.12.16 and lower break within the tests if target is set to 11
           if isScala211(partialVersion) ||
             scalaVersion.value == "2.12.16" ||
             isScala212(partialVersion) && V.deprecatedScala2Versions
@@ -843,8 +843,6 @@ lazy val slow = project
   .settings(
     testSettings,
     sharedSettings,
-    // Test / javaOptions += "-Xmx2G",
-    // Test / javaOptions += "-XX:+HeapDumpOnOutOfMemoryError",
     Test / testOnly := (Test / testOnly)
       .dependsOn((`sbt-metals` / publishLocal), publishBinaryMtags)
       .evaluated,

metals/src/main/scala/scala/meta/internal/metals/BuildServerConnection.scala

@@ -333,7 +333,8 @@ class BuildServerConnection private (
   }
 
   def buildTargetJvmClasspath(
-      params: JvmCompileClasspathParams
+      params: JvmCompileClasspathParams,
+      cancelPromise: Promise[Unit],
   ): Future[JvmCompileClasspathResult] = {
     val resultOnScalaOptionsUnsupported = new JvmCompileClasspathResult(
       List.empty[JvmCompileClasspathItem].asJava
@@ -346,10 +347,12 @@ class BuildServerConnection private (
             "Jvm compile classpath request not supported by server",
           )
         )
-      register(
+      val completable = register(
         server => server.buildTargetJvmCompileClasspath(params),
         onFail,
-      ).asScala
+      )
+      cancelPromise.future.map(_ => completable.cancel(true))
+      completable.asScala
     } else Future.successful(resultOnScalaOptionsUnsupported)
   }
 

metals/src/main/scala/scala/meta/internal/metals/BuildTargetInfo.scala

@@ -129,13 +129,28 @@ class BuildTargetInfo(buildTargets: BuildTargets) {
       )
     }
 
-    val scalaClassPath = scalaInfo
-      .flatMap(_.classpath) match {
-      case None => List("<unresolved>")
-      case Some(classpath) =>
-        getClassPath(classpath.map(_.toAbsolutePath), targetId)
+    val scalaClassPath =
+      scalaInfo.flatMap(_.classpath).getOrElse(Nil).map(_.toAbsolutePath)
+    val javaClassPath =
+      javaInfo.flatMap(_.classpath).getOrElse(Nil).map(_.toAbsolutePath)
+    if (scalaClassPath == javaClassPath)
+      if (scalaClassPath.isEmpty)
+        List("<unresolved>")
+      else
+        output ++= getSection(
+          "Classpath",
+          getClassPath(scalaClassPath, targetId),
+        )
+    else {
+      output ++= getSection(
+        "Java Classpath",
+        getClassPath(javaClassPath, targetId),
+      )
+      output ++= getSection(
+        "Scala Classpath",
+        getClassPath(scalaClassPath, targetId),
+      )
     }
-    output ++= getSection("Classpath", scalaClassPath)
     output += ""
     output.mkString(System.lineSeparator())
   }

metals/src/main/scala/scala/meta/internal/metals/BuildTargets.scala

@@ -9,6 +9,7 @@ import scala.annotation.tailrec
 import scala.collection.mutable
 import scala.concurrent.ExecutionContext
 import scala.concurrent.Future
+import scala.concurrent.Promise
 import scala.util.control.NonFatal
 
 import scala.meta.internal.io.PathIO
@@ -117,9 +118,10 @@ final class BuildTargets private (
     data.fromOptions(_.javaTarget(id))
 
   def fullClasspath(
-      id: BuildTargetIdentifier
+      id: BuildTargetIdentifier,
+      cancelPromise: Promise[Unit],
   )(implicit ec: ExecutionContext): Option[Future[List[AbsolutePath]]] =
-    targetClasspath(id).map { lazyClasspath =>
+    targetClasspath(id, cancelPromise).map { lazyClasspath =>
       lazyClasspath.map { classpath =>
         classpath.map(_.toAbsolutePath).collect {
           case path if path.isJar || path.isDirectory =>
@@ -134,9 +136,10 @@ final class BuildTargets private (
     data.fromOptions(_.targetJarClasspath(id))
 
   def targetClasspath(
-      id: BuildTargetIdentifier
+      id: BuildTargetIdentifier,
+      cancelPromise: Promise[Unit],
   )(implicit executionContext: ExecutionContext): Option[Future[List[String]]] =
-    data.fromOptions(_.targetClasspath(id))
+    data.fromOptions(_.targetClasspath(id, cancelPromise))
 
   def targetClassDirectories(
       id: BuildTargetIdentifier

metals/src/main/scala/scala/meta/internal/metals/CompilerConfiguration.scala

@@ -7,7 +7,9 @@ import java.{util => ju}
 import scala.concurrent.Await
 import scala.concurrent.ExecutionContextExecutorService
 import scala.concurrent.Future
+import scala.concurrent.Promise
 import scala.concurrent.duration.Duration
+import scala.util.Success
 import scala.util.Try
 import scala.util.control.NonFatal
 
@@ -42,6 +44,7 @@ class CompilerConfiguration(
 
   sealed trait MtagsPresentationCompiler {
     def await: PresentationCompiler
+    def shutdown(): Unit
   }
 
   case class StandaloneCompiler(
@@ -61,6 +64,7 @@ class CompilerConfiguration(
         symbolSearch,
       )
 
+    def shutdown(): Unit = standalone.shutdown()
     def await: PresentationCompiler = standalone
   }
 
@@ -91,18 +95,19 @@ class CompilerConfiguration(
     protected val presentationCompilerRef =
       new ju.concurrent.atomic.AtomicReference[PresentationCompiler]()
 
-    private val wasResolved = new ju.concurrent.atomic.AtomicBoolean(false)
+    protected val cancelCompilerPromise: Promise[Unit] = Promise[Unit]()
     protected val presentationCompilerFuture: Future[PresentationCompiler] =
       buildTargets
-        .targetClasspath(buildTargetId)
+        .targetClasspath(buildTargetId, cancelCompilerPromise)
         .getOrElse(Future.successful(Nil))
         .map { classpath =>
           // set wasResolved to avoid races on timeout below
-          wasResolved.set(true)
-          // Request finished, we can remove and shut down the fallback
-          Option(presentationCompilerRef.getAndSet(null)).foreach(_.shutdown())
           val classpathSeq = classpath.toAbsoluteClasspath.map(_.toNIO).toSeq
-          newCompiler(classpathSeq)
+          val result = newCompiler(classpathSeq)
+          // Request finished, we can remove and shut down the fallback
+          Option(presentationCompilerRef.getAndSet(result))
+            .foreach(_.shutdown())
+          result
         }
 
     def await: PresentationCompiler = {
@@ -116,24 +121,33 @@ class CompilerConfiguration(
             presentationCompilerFuture,
             Duration(compilerConfig.timeoutDelay, compilerConfig.timeoutUnit),
           )
-          presentationCompilerRef.set(result)
           result
         }
       } catch {
         case _: ju.concurrent.TimeoutException =>
           scribe.warn(
             s"Still waiting for information about classpath, using standalone compiler for now"
           )
-          val pc = presentationCompilerRef.updateAndGet { old =>
+          this.synchronized {
+            val old = presentationCompilerRef.get()
             if (old != null) old
-            else if (!wasResolved.get()) fallback
-            else null
+            else {
+              val newFallback = fallback
+              presentationCompilerRef.set(newFallback)
+              newFallback
+            }
           }
-          if (pc != null) pc
-          // null is only returned if classpath was resolved while we were waiting and there was a race
-          else await
       }
     }
+
+    def shutdown(): Unit = {
+      cancelCompilerPromise.trySuccess(())
+      presentationCompilerFuture.onComplete {
+        case Success(value) => value.shutdown()
+        case _ =>
+      }
+      Option(presentationCompilerRef.get()).foreach(_.shutdown())
+    }
   }
 
   case class ScalaLazyCompiler(
@@ -268,11 +282,25 @@ class CompilerConfiguration(
       workspaceFallback,
     )
   } catch {
-    case NonFatal(e) => {
+    case NonFatal(error) => {
       scribe.error(
         "Could not create standalone symbol search, please report an issue.",
-        e,
+        error,
       )
+      val report =
+        Report(
+          "standalone-serach-error",
+          s"""|occurred while creating classpath search
+              |
+              |classpath:
+              |${classpath.mkString(",")}
+              |
+              |sources:
+              |${sources.mkString(",")}
+              |""".stripMargin,
+          error,
+        )
+      rc.unsanitized.create(report)
       EmptySymbolSearch
     }
   }

metals/src/main/scala/scala/meta/internal/metals/Compilers.scala

@@ -156,8 +156,8 @@ class Compilers(
     cache.values.count(_.await.isLoaded())
 
   override def cancel(): Unit = {
-    Cancelable.cancelEach(cache.values)(_.await.shutdown())
-    Cancelable.cancelEach(worksheetsCache.values)(_.await.shutdown())
+    Cancelable.cancelEach(cache.values)(_.shutdown())
+    Cancelable.cancelEach(worksheetsCache.values)(_.shutdown())
     cache.clear()
     worksheetsCache.clear()
     worksheetsDigests.clear()

metals/src/main/scala/scala/meta/internal/metals/FileDecoderProvider.scala

@@ -9,6 +9,7 @@ import javax.annotation.Nullable
 import scala.annotation.tailrec
 import scala.concurrent.ExecutionContext
 import scala.concurrent.Future
+import scala.concurrent.Promise
 import scala.util.Failure
 import scala.util.Properties
 import scala.util.Success
@@ -597,7 +598,7 @@ final class FileDecoderProvider(
           .map(_.id)
       )
     buildTarget
-      .flatMap(id => buildTargets.targetClasspath(id))
+      .flatMap(id => buildTargets.targetClasspath(id, Promise[Unit]()))
       .getOrElse(Future.successful(Nil))
       .map(_.map(_.toAbsolutePath))
       .flatMap { classpaths =>

metals/src/main/scala/scala/meta/internal/metals/JvmTarget.scala

@@ -1,8 +1,44 @@
 package scala.meta.internal.metals
 
+import scala.meta.internal.metals.MetalsEnrichments._
+import scala.meta.io.AbsolutePath
+
 trait JvmTarget {
 
+  /**
+   * If the build server supports lazy classpath resolution, we will
+   * not get any classpath data eagerly and we should not
+   * use this endpoint. It should only be used as a fallback.
+   *
+   * This is due to the fact that we don't request classpath as it
+   * can be resonably expensive.
+   *
+   * @return non empty classpath only if it was resolved prior
+   */
   def classpath: Option[List[String]]
 
   def classDirectory: String
+
+  /**
+   * This method collects jars from classpath defined in scalacOptions.
+   *
+   * If the build server supports lazy classpath resolution, we will
+   * not get any classpath data eagerly and we should not
+   * use this endpoint. It should only be used as a fallback.
+   *
+   * This is due to the fact that we don't request classpath as it
+   * can be resonably expensive.
+   *
+   * We should use the buildTargetDependencyModules information
+   * from the indexer instead.
+   *
+   * @return non empty classpath jar list if it was resolved prior
+   */
+  def jarClasspath: Option[List[AbsolutePath]] =
+    classpath.map(collectJars)
+
+  private def collectJars(paths: List[String]) =
+    paths
+      .filter(_.endsWith(".jar"))
+      .map(_.toAbsolutePath)
 }

metals/src/main/scala/scala/meta/internal/metals/ScalaTarget.scala

@@ -111,29 +111,6 @@ case class ScalaTarget(
     else
       Some(scalac.getClasspath().asScala.toList)
 
-  /**
-   * This method collects jars from classpath defined in scalacOptions.
-   *
-   * If the build server supports lazy classpath resolution, we will
-   * not get any classpath data eagerly and we should not
-   * use this endpoint. It should only be used as a fallback.
-   *
-   * This is due to the fact that we don't request classpath as it
-   * can be resonably expensive.
-   *
-   * We should use the buildTargetDependencyModules information
-   * from the indexer instead.
-   *
-   * @return non empty classpath jar list if it was resolved prior
-   */
-  def jarClasspath: Option[List[AbsolutePath]] =
-    classpath.map(collectJars)
-
-  private def collectJars(paths: List[String]) =
-    paths
-      .filter(_.endsWith(".jar"))
-      .map(_.toAbsolutePath)
-
   def classDirectory: String = scalac.getClassDirectory()
 
   def scalaVersion: String = scalaInfo.getScalaVersion()

metals/src/main/scala/scala/meta/internal/metals/ScalafixProvider.scala

@@ -8,6 +8,7 @@ import java.{util => ju}
 import scala.collection.concurrent.TrieMap
 import scala.concurrent.ExecutionContext
 import scala.concurrent.Future
+import scala.concurrent.Promise
 import scala.util.Failure
 import scala.util.Random
 import scala.util.Success
@@ -76,9 +77,7 @@ case class ScalafixProvider(
         Future.sequence(testEvaluation)
       }.flatten
         .map { results =>
-          results.forall(_.isSuccessful())
-        }
-        .map { evaluated =>
+          val evaluated = results.forall(_.isSuccessful())
           if (!evaluated) scribe.debug("Could not warm up Scalafix")
 
         }
@@ -381,7 +380,7 @@ case class ScalafixProvider(
     // It seems that Scalafix ignores the targetroot parameter and searches the classpath
     // Prepend targetroot to make sure that it's picked up first always
     val lazyClasspath = buildTargets
-      .fullClasspath(scalaTarget.id)
+      .fullClasspath(scalaTarget.id, Promise[Unit]())
       .getOrElse(Future.successful(Nil))
       .map { classpath =>
         (targetRoot.toList ++ classpath.map(_.toNIO)).asJava