improvement: Recreate classloader if scalafix classloading fails

I wasn't able to figure out why this might be happening for people, but usually this happened if scalafix was invoked at a wrong time, running it later would work. I also added reporting and a potential fix for Scalafix cache, since we could have a race updating caches and with getOrElseUpdate we are at least sure the same exact result will be returned.
scalameta · Jun 18, 2024 · dc572a5 · dc572a5
1 parent b8d7199
commit dc572a5
Showing 1 changed file with 61 additions and 29 deletions.
diff --git a/metals/src/main/scala/scala/meta/internal/metals/ScalafixProvider.scala b/metals/src/main/scala/scala/meta/internal/metals/ScalafixProvider.scala
@@ -49,7 +49,7 @@ case class ScalafixProvider(
     interactive: InteractiveSemanticdbs,
     tables: Tables,
     buildHasErrors: AbsolutePath => Boolean,
-)(implicit ec: ExecutionContext) {
+)(implicit ec: ExecutionContext, rc: ReportContext) {
   import ScalafixProvider._
   private val scalafixCache = TrieMap.empty[ScalaBinaryVersion, Scalafix]
   private val rulesClassloaderCache =
@@ -329,6 +329,7 @@ case class ScalafixProvider(
       produceSemanticdb: Boolean,
       rules: List[String],
       suggestConfigAmend: Boolean = true,
+      shouldRetry: Boolean = true,
   ): Future[ScalafixEvaluation] = {
     val isScala3 = ScalaVersions.isScala3Version(scalaTarget.scalaVersion)
     val isSource3 = scalaTarget.scalac.getOptions().contains("-Xsource:3")
@@ -399,7 +400,7 @@ case class ScalafixProvider(
         list
       }
 
-      for {
+      val evalFuture = for {
         classpath <- lazyClasspath
         confFile <- getScalafixConf(
           isSource3,
@@ -430,6 +431,43 @@ case class ScalafixProvider(
         }
         evaluated
       }
+      evalFuture.recoverWith {
+        case serviceError: java.util.ServiceConfigurationError =>
+          scribe.error(
+            "Scalafix classloading error, retrying with new classloader",
+            serviceError,
+          )
+          val classpath =
+            urlClassLoaderWithExternalRule.getURLs().mkString("\n")
+          val report =
+            Report(
+              "scalafix-classloading-error",
+              s"""|Could not load scalafix rules.
+                  |
+                  |classpath:
+                  |${classpath}
+                  |
+                  |file: $file
+                  |
+                  |scalaVersion: ${scalaTarget.scalaVersion}
+                  |
+                  |""".stripMargin,
+              serviceError,
+            )
+          rc.incognito.create(report)
+          if (shouldRetry) {
+            rulesClassloaderCache.remove(scalafixRulesKey)
+            scalafixEvaluate(
+              file,
+              scalaTarget,
+              inBuffers,
+              produceSemanticdb,
+              rules,
+              suggestConfigAmend,
+              shouldRetry = false,
+            )
+          } else Future.failed(serviceError)
+      }
     }
     result.flatten
   }
@@ -530,19 +568,18 @@ case class ScalafixProvider(
 
   private def getScalafix(
       scalaBinaryVersion: ScalaBinaryVersion
-  ): Future[Scalafix] = {
-    scalafixCache.get(scalaBinaryVersion) match {
-      case Some(value) => Future.successful(value)
-      case None =>
+  ): Future[Scalafix] = Future {
+    scalafixCache.getOrElseUpdate(
+      scalaBinaryVersion, {
         workDoneProgress.trackBlocking("Downloading scalafix") {
           val scalafix =
-            if (scalaBinaryVersion == "2.11") Future(scala211Fallback)
+            if (scalaBinaryVersion == "2.11") scala211Fallback
             else
-              Future(Scalafix.fetchAndClassloadInstance(scalaBinaryVersion))
-          scalafix.foreach(api => scalafixCache.update(scalaBinaryVersion, api))
+              Scalafix.fetchAndClassloadInstance(scalaBinaryVersion)
           scalafix
         }
-    }
+      },
+    )
 
   }
 
@@ -565,10 +602,9 @@ case class ScalafixProvider(
   private def getRuleClassLoader(
       scalfixRulesKey: ScalafixRulesClasspathKey,
       scalafixClassLoader: ClassLoader,
-  ): Future[URLClassLoader] = {
-    rulesClassloaderCache.get(scalfixRulesKey) match {
-      case Some(value) => Future.successful(value)
-      case None =>
+  ): Future[URLClassLoader] = Future {
+    rulesClassloaderCache.getOrElseUpdate(
+      scalfixRulesKey, {
         workDoneProgress.trackBlocking(
           "Downloading scalafix rules' dependencies"
         ) {
@@ -585,22 +621,18 @@ case class ScalafixProvider(
               )
             else None
 
-          val allRules =
-            Future(
-              Embedded.rulesClasspath(
-                rulesDependencies.toList ++ organizeImportRule
-              )
-            ).map { paths =>
-              val classloader = Embedded.toClassLoader(
-                Classpath(paths.map(AbsolutePath(_))),
-                scalafixClassLoader,
-              )
-              rulesClassloaderCache.update(scalfixRulesKey, classloader)
-              classloader
-            }
-          allRules
+          val paths =
+            Embedded.rulesClasspath(
+              rulesDependencies.toList ++ organizeImportRule
+            )
+          val classloader = Embedded.toClassLoader(
+            Classpath(paths.map(AbsolutePath(_))),
+            scalafixClassLoader,
+          )
+          classloader
         }
-    }
+      },
+    )
   }
 
   private def isUnsaved(fromBuffers: String, fromFile: String): Boolean = {