Merge pull request #1384 from jrudolph/fix-redownloading

Use a simple cache based on futures to avoid redownloading on concurrent usage

scalafmt-dynamic/src/main/scala/org/scalafmt/dynamic/ScalafmtDynamic.scala

@@ -8,10 +8,10 @@ import com.typesafe.config.{ConfigException, ConfigFactory}
 import org.scalafmt.dynamic.ScalafmtDynamic.{FormatEval, FormatResult}
 import org.scalafmt.dynamic.ScalafmtDynamicDownloader._
 import org.scalafmt.dynamic.exceptions._
+import org.scalafmt.dynamic.utils.ReentrantCache
 import org.scalafmt.interfaces._
 
-import scala.collection.concurrent.TrieMap
-import scala.collection.mutable
+import scala.concurrent.ExecutionContext
 import scala.util.Try
 import scala.util.control.NonFatal
 
@@ -20,25 +20,31 @@ final case class ScalafmtDynamic(
     respectVersion: Boolean,
     respectExcludeFilters: Boolean,
     defaultVersion: String,
-    fmtsCache: mutable.Map[String, ScalafmtReflect],
+    formatCache: ReentrantCache[String, FormatEval[ScalafmtReflect]],
     cacheConfigs: Boolean,
-    configsCache: mutable.Map[Path, (ScalafmtReflectConfig, FileTime)]
+    configsCache: ReentrantCache[Path, FormatEval[
+      (ScalafmtReflectConfig, FileTime)
+    ]]
 ) extends Scalafmt {
 
   def this() = this(
     ConsoleScalafmtReporter,
     true,
     true,
     BuildInfo.stable,
-    TrieMap.empty,
+    ReentrantCache(),
     true,
-    TrieMap.empty
+    ReentrantCache()
   )
 
-  override def clear(): Unit = {
-    fmtsCache.values.foreach(_.classLoader.close())
-    fmtsCache.clear()
-  }
+  override def clear(): Unit =
+    formatCache
+      .clear()
+      .foreach(
+        _.foreach(_.right.foreach(_.classLoader.close()))(
+          ExecutionContext.global
+        )
+      )
 
   override def withReporter(reporter: ScalafmtReporter): ScalafmtDynamic =
     copy(reporter = reporter)
@@ -108,19 +114,17 @@ final case class ScalafmtDynamic(
       Left(ScalafmtDynamicError.ConfigDoesNotExist(configPath))
     } else if (cacheConfigs) {
       val currentTimestamp: FileTime = Files.getLastModifiedTime(configPath)
-      configsCache.get(configPath) match {
-        case Some((config, lastModified))
-            if lastModified.compareTo(currentTimestamp) == 0 =>
-          Right(config)
-        case _ =>
-          for {
-            config <- resolveConfigWithScalafmt(configPath)
-          } yield {
-            configsCache(configPath) = (config, currentTimestamp)
+      configsCache
+        .getOrAddToCache(
+          configPath,
+          _.right.exists(_._2.compareTo(currentTimestamp) != 0)
+        ) { () =>
+          resolveConfigWithScalafmt(configPath).map { config =>
             reporter.parsedConfig(configPath, config.version)
-            config
+            (config, currentTimestamp)
           }
-      }
+        }
+        .map(_._1)
     } else {
       resolveConfigWithScalafmt(configPath)
     }
@@ -149,33 +153,23 @@ final case class ScalafmtDynamic(
     }
   }
 
-  // TODO: there can be issues if resolveFormatter is called multiple times (e.g. formatter is called twice)
-  //  in such cases download process can be started multiple times,
-  //  possible solution: keep information about download process in fmtsCache
   private def resolveFormatter(
       configPath: Path,
       version: String
   ): FormatEval[ScalafmtReflect] = {
-    fmtsCache.get(version) match {
-      case Some(value) =>
-        Right(value)
-      case None =>
-        val writer = reporter.downloadWriter()
-        val downloader = new ScalafmtDynamicDownloader(writer)
-        downloader
-          .download(version)
-          .left
-          .map {
-            case DownloadResolutionError(v, _) =>
-              ScalafmtDynamicError.CannotDownload(configPath, v, None)
-            case DownloadUnknownError(v, cause) =>
-              ScalafmtDynamicError.CannotDownload(configPath, v, Option(cause))
-          }
-          .flatMap(resolveClassPath(configPath, _))
-          .map { scalafmt: ScalafmtReflect =>
-            fmtsCache(version) = scalafmt
-            scalafmt
-          }
+    formatCache.getOrAddToCache(version) { () =>
+      val writer = reporter.downloadWriter()
+      val downloader = new ScalafmtDynamicDownloader(writer)
+      downloader
+        .download(version)
+        .left
+        .map {
+          case DownloadResolutionError(v, _) =>
+            ScalafmtDynamicError.CannotDownload(configPath, v, None)
+          case DownloadUnknownError(v, cause) =>
+            ScalafmtDynamicError.CannotDownload(configPath, v, Option(cause))
+        }
+        .flatMap(resolveClassPath(configPath, _))
     }
   }
 

scalafmt-dynamic/src/main/scala/org/scalafmt/dynamic/utils/ReentrantCache.scala

@@ -0,0 +1,58 @@
+package org.scalafmt.dynamic.utils
+
+import scala.annotation.tailrec
+import scala.concurrent.Await
+import scala.concurrent.Future
+import scala.concurrent.Promise
+import scala.concurrent.duration._
+import scala.util.Try
+
+class ReentrantCache[K, V] {
+  private[this] var cache: Map[K, Future[V]] = Map.empty
+
+  @tailrec
+  final def getOrAddToCache(key: K, shouldEvict: V => Boolean = _ => false)(
+      get: () => V
+  ): V =
+    synchronized { // try to exit quickly from synchronized block
+      cache.get(key) match {
+        case Some(fut) => Right(fut)
+        case None =>
+          val p = Promise[V]()
+          cache += key -> p.future
+          Left(p)
+      }
+    } match {
+      case Right(fut) =>
+        // we set the timeout to 10 minutes because
+        // we can't expect everybody to have the same internet connection speed.
+        //
+        // get or wait for other thread to finish download
+        val result = Await.result(fut, 10.minute)
+
+        if (shouldEvict(result)) {
+          synchronized {
+            cache -= key
+          }
+          getOrAddToCache(key, shouldEvict)(get)
+        } else
+          result
+      case Left(p) =>
+        val result = Try(get())
+        p.complete(result)
+        result.get
+    }
+
+  def clear(): Iterable[Future[V]] =
+    synchronized {
+      val oldValues = cache.values
+      cache = Map.empty
+      oldValues
+    }
+
+  def getFromCache(key: K): Option[V] =
+    cache.get(key).map(Await.result(_, 10.minute))
+}
+object ReentrantCache {
+  def apply[K, V](): ReentrantCache[K, V] = new ReentrantCache[K, V]
+}

scalafmt-dynamic/src/test/scala/tests/DynamicSuite.scala

@@ -362,9 +362,9 @@ class DynamicSuite extends FunSuite with DiffAssertions {
     f.setVersion(version)
     f.assertFormat()
 
-    val reflect = f.dynamic.fmtsCache.get(version)
+    val reflect = f.dynamic.formatCache.getFromCache(version)
     assert(reflect.nonEmpty)
-    assert(reflect.get.intellijScalaFmtConfig.nonEmpty)
+    assert(reflect.get.right.get.intellijScalaFmtConfig.nonEmpty)
   }
 
   checkExhaustive("continuation-indent-callSite-and-defnSite") { (f, version) =>
@@ -408,7 +408,9 @@ class DynamicSuite extends FunSuite with DiffAssertions {
         """.stripMargin
     )
     f.assertFormat()
-    val configOpt = f.dynamic.configsCache.get(f.config).map(_._1)
+    val configOpt = f.dynamic.configsCache
+      .getFromCache(f.config)
+      .flatMap(_.toOption.map(_._1))
     assert(configOpt.nonEmpty)
     val config = configOpt.get
     assert(config.hasRewriteRules)