Don't serve stale JAR metadata from cache in classpath implementation

Use the last modified timestamp and the file inode to help detect
when the file has been overwritten (as is common in SBT builds
with `exportJars := true`, or when using snapshot dependencies).

Fixes scala/bug#10295

Author: retronym

src/compiler/scala/tools/nsc/classpath/ZipAndJarFileLookupFactory.scala

@@ -5,38 +5,32 @@ package scala.tools.nsc.classpath
 
 import java.io.File
 import java.net.URL
+import java.nio.file.Files
+import java.nio.file.attribute.{BasicFileAttributes, FileTime}
 
 import scala.annotation.tailrec
-import scala.reflect.io.{ AbstractFile, FileZipArchive, ManifestResources }
+import scala.reflect.io.{AbstractFile, FileZipArchive, ManifestResources}
 import scala.tools.nsc.util.{ClassPath, ClassRepresentation}
 import scala.tools.nsc.Settings
 import FileUtils._
 
 /**
  * A trait providing an optional cache for classpath entries obtained from zip and jar files.
- * It's possible to create such a cache assuming that entries in such files won't change (at
- * least will be the same each time we'll load classpath during the lifetime of JVM process)
- * - unlike class and source files in directories, which can be modified and recompiled.
  * It allows us to e.g. reduce significantly memory used by PresentationCompilers in Scala IDE
  * when there are a lot of projects having a lot of common dependencies.
  */
 sealed trait ZipAndJarFileLookupFactory {
-  private val cache = collection.mutable.Map.empty[AbstractFile, ClassPath]
+  private val cache = new FileBasedCache[ClassPath]
 
   def create(zipFile: AbstractFile, settings: Settings): ClassPath = {
-    if (settings.YdisableFlatCpCaching) createForZipFile(zipFile)
+    if (settings.YdisableFlatCpCaching || zipFile.file == null) createForZipFile(zipFile)
     else createUsingCache(zipFile, settings)
   }
 
   protected def createForZipFile(zipFile: AbstractFile): ClassPath
 
-  private def createUsingCache(zipFile: AbstractFile, settings: Settings): ClassPath = cache.synchronized {
-    def newClassPathInstance = {
-      if (settings.verbose || settings.Ylogcp)
-        println(s"$zipFile is not yet in the classpath cache")
-      createForZipFile(zipFile)
-    }
-    cache.getOrElseUpdate(zipFile, newClassPathInstance)
+  private def createUsingCache(zipFile: AbstractFile, settings: Settings): ClassPath = {
+    cache.getOrCreate(zipFile.file.toPath, () => createForZipFile(zipFile))
   }
 }
 
@@ -181,3 +175,29 @@ object ZipAndJarSourcePathFactory extends ZipAndJarFileLookupFactory {
 
   override protected def createForZipFile(zipFile: AbstractFile): ClassPath = ZipArchiveSourcePath(zipFile.file)
 }
+
+final class FileBasedCache[T] {
+  private case class Stamp(lastModified: FileTime, fileKey: Object)
+  private val cache = collection.mutable.Map.empty[java.nio.file.Path, (Stamp, T)]
+
+  def getOrCreate(path: java.nio.file.Path, create: () => T): T = cache.synchronized {
+    val attrs = Files.readAttributes(path, classOf[BasicFileAttributes])
+    val lastModified = attrs.lastModifiedTime()
+    // only null on some platforms, but that's okay, we just use the last modified timestamp as our stamp
+    val fileKey = attrs.fileKey()
+    val stamp = Stamp(lastModified, fileKey)
+    cache.get(path) match {
+      case Some((cachedStamp, cached)) if cachedStamp == stamp => cached
+      case _ =>
+        val value = create()
+        cache.put(path, (stamp, value))
+        value
+    }
+  }
+
+  def clear(): Unit = cache.synchronized {
+    // TODO support closing
+    // cache.valuesIterator.foreach(_.close())
+    cache.clear()
+  }
+}

test/junit/scala/tools/nsc/classpath/ZipAndJarFileLookupFactoryTest.scala

@@ -0,0 +1,64 @@
+package scala.tools.nsc
+package classpath
+
+import org.junit.Test
+import java.nio.file._
+import java.nio.file.attribute.FileTime
+import scala.reflect.io.AbstractFile
+
+class ZipAndJarFileLookupFactoryTest {
+  @Test def cacheInvalidation(): Unit = {
+    val f = Files.createTempFile("test-", ".jar")
+    Files.delete(f)
+    val g = new scala.tools.nsc.Global(new scala.tools.nsc.Settings())
+    assert(!g.settings.YdisableFlatCpCaching.value) // we're testing with our JAR metadata caching enabled.
+    def createCp = ZipAndJarClassPathFactory.create(AbstractFile.getFile(f.toFile), g.settings)
+    try {
+      createZip(f, Array(), "p1/C.class")
+      createZip(f, Array(), "p2/X.class")
+      createZip(f, Array(), "p3/Y.class")
+      val cp1 = createCp
+      assert(cp1.findClass("p1.C").isDefined)
+
+      // We expect get a cache hit as the underlying zip hasn't changed
+      val cp2 = createCp
+      assert(cp2 eq cp1)
+
+      // check things work after the cache hit
+      cp1.findClassFile("p2.X").get.toByteArray
+
+      val lastMod1 = Files.getLastModifiedTime(f)
+      // Create a new zip at the same path with different contents and last modified
+      Files.delete(f)
+      createZip(f, Array(), "p1/D.class")
+      Files.setLastModifiedTime(f, FileTime.fromMillis(lastMod1.toMillis + 2000))
+
+      // Our classpath cache should create a new instance
+      val cp3 = createCp
+      assert(cp1 ne cp3, (System.identityHashCode(cp1), System.identityHashCode(cp3)))
+      // And that instance should see D, not C, in package p1.
+      assert(cp3.findClass("p1.C").isEmpty)
+      assert(cp3.findClass("p1.D").isDefined)
+    } finally Files.delete(f)
+  }
+
+  def createZip(zipLocation: Path, content: Array[Byte], internalPath: String): Unit = {
+    val env = new java.util.HashMap[String, String]()
+    env.put("create", String.valueOf(Files.notExists(zipLocation)))
+    val fileUri = zipLocation.toUri
+    val zipUri = new java.net.URI("jar:" + fileUri.getScheme, fileUri.getPath, null)
+    val zipfs = FileSystems.newFileSystem(zipUri, env)
+    try {
+      try {
+        val internalTargetPath = zipfs.getPath(internalPath)
+        Files.createDirectories(internalTargetPath.getParent)
+        Files.write(internalTargetPath, content)
+      } finally {
+        if (zipfs != null) zipfs.close()
+      }
+    } finally {
+      zipfs.close()
+    }
+  }
+}
+