From d810ffc0bd645a3773d9dab07a54e5c61390d0dd Mon Sep 17 00:00:00 2001 From: Guillaume Martres Date: Tue, 7 Jan 2020 18:59:07 +0100 Subject: [PATCH 1/2] Fix #7852: avoid reading stale .tasty files from jars MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Somehow, if we don't close the classloader used to read .tasty files from jars, a subsequent compilation run might end up reading a stale version of the .tasty file which does not exist on disk anymore. I don't understand why this happens since we don't reuse the classloader itself, but that's life sometimes ¯\_(ツ)_/¯ --- .../dotc/core/classfile/ClassfileParser.scala | 35 +++++++++++-------- .../export-jars/changes/{B.scala => B1.scala} | 0 .../export-jars2/a/A.scala | 1 + .../export-jars2/b/B.scala | 4 +++ .../export-jars2/build.sbt | 10 ++++++ .../export-jars2/changes/A1.scala | 2 ++ .../export-jars2/changes/B1.scala | 5 +++ .../project/DottyInjectedPlugin.scala | 12 +++++++ .../export-jars2/project/plugins.sbt | 1 + .../source-dependencies/export-jars2/test | 7 ++++ 10 files changed, 63 insertions(+), 14 deletions(-) rename sbt-dotty/sbt-test/source-dependencies/export-jars/changes/{B.scala => B1.scala} (100%) create mode 100644 sbt-dotty/sbt-test/source-dependencies/export-jars2/a/A.scala create mode 100644 sbt-dotty/sbt-test/source-dependencies/export-jars2/b/B.scala create mode 100644 sbt-dotty/sbt-test/source-dependencies/export-jars2/build.sbt create mode 100644 sbt-dotty/sbt-test/source-dependencies/export-jars2/changes/A1.scala create mode 100644 sbt-dotty/sbt-test/source-dependencies/export-jars2/changes/B1.scala create mode 100644 sbt-dotty/sbt-test/source-dependencies/export-jars2/project/DottyInjectedPlugin.scala create mode 100644 sbt-dotty/sbt-test/source-dependencies/export-jars2/project/plugins.sbt create mode 100644 sbt-dotty/sbt-test/source-dependencies/export-jars2/test diff --git a/compiler/src/dotty/tools/dotc/core/classfile/ClassfileParser.scala b/compiler/src/dotty/tools/dotc/core/classfile/ClassfileParser.scala index 2813a57e5acb..6bd476e4b650 100644 --- a/compiler/src/dotty/tools/dotc/core/classfile/ClassfileParser.scala +++ b/compiler/src/dotty/tools/dotc/core/classfile/ClassfileParser.scala @@ -780,22 +780,29 @@ class ClassfileParser( Array.empty case Some(jar: ZipArchive) => // We are in a jar val cl = new URLClassLoader(Array(jar.jpath.toUri.toURL), /*parent =*/ null) - val path = classfile.path.stripSuffix(".class") + ".tasty" - val stream = cl.getResourceAsStream(path) - if (stream != null) { - val tastyOutStream = new ByteArrayOutputStream() - val buffer = new Array[Byte](1024) - var read = stream.read(buffer, 0, buffer.length) - while (read != -1) { - tastyOutStream.write(buffer, 0, read) - read = stream.read(buffer, 0, buffer.length) + try { + val path = classfile.path.stripSuffix(".class") + ".tasty" + val stream = cl.getResourceAsStream(path) + if (stream != null) { + val tastyOutStream = new ByteArrayOutputStream() + val buffer = new Array[Byte](1024) + var read = stream.read(buffer, 0, buffer.length) + while (read != -1) { + tastyOutStream.write(buffer, 0, read) + read = stream.read(buffer, 0, buffer.length) + } + tastyOutStream.flush() + tastyOutStream.toByteArray + } + else { + ctx.error(s"Could not find $path in $jar") + Array.empty } - tastyOutStream.flush() - tastyOutStream.toByteArray } - else { - ctx.error(s"Could not find $path in $jar") - Array.empty + finally { + // If we don't close the classloader, spooky things happen (see + // scripted test source-dependencies/export-jars2). + cl.close() } case _ => val plainFile = new PlainFile(io.File(classfile.jpath).changeExtension("tasty")) diff --git a/sbt-dotty/sbt-test/source-dependencies/export-jars/changes/B.scala b/sbt-dotty/sbt-test/source-dependencies/export-jars/changes/B1.scala similarity index 100% rename from sbt-dotty/sbt-test/source-dependencies/export-jars/changes/B.scala rename to sbt-dotty/sbt-test/source-dependencies/export-jars/changes/B1.scala diff --git a/sbt-dotty/sbt-test/source-dependencies/export-jars2/a/A.scala b/sbt-dotty/sbt-test/source-dependencies/export-jars2/a/A.scala new file mode 100644 index 000000000000..83d15dc739b5 --- /dev/null +++ b/sbt-dotty/sbt-test/source-dependencies/export-jars2/a/A.scala @@ -0,0 +1 @@ +class A diff --git a/sbt-dotty/sbt-test/source-dependencies/export-jars2/b/B.scala b/sbt-dotty/sbt-test/source-dependencies/export-jars2/b/B.scala new file mode 100644 index 000000000000..f1c7a7bec301 --- /dev/null +++ b/sbt-dotty/sbt-test/source-dependencies/export-jars2/b/B.scala @@ -0,0 +1,4 @@ +class B { + val x = new A +} + diff --git a/sbt-dotty/sbt-test/source-dependencies/export-jars2/build.sbt b/sbt-dotty/sbt-test/source-dependencies/export-jars2/build.sbt new file mode 100644 index 000000000000..a7b425088f99 --- /dev/null +++ b/sbt-dotty/sbt-test/source-dependencies/export-jars2/build.sbt @@ -0,0 +1,10 @@ +lazy val a = Project("a", file("a")) + .settings( + exportJars := true + ) + +lazy val b = Project("b", file("b")) + .dependsOn(a) + .settings( + exportJars := true + ) diff --git a/sbt-dotty/sbt-test/source-dependencies/export-jars2/changes/A1.scala b/sbt-dotty/sbt-test/source-dependencies/export-jars2/changes/A1.scala new file mode 100644 index 000000000000..c1b9f93b4afc --- /dev/null +++ b/sbt-dotty/sbt-test/source-dependencies/export-jars2/changes/A1.scala @@ -0,0 +1,2 @@ + +class A diff --git a/sbt-dotty/sbt-test/source-dependencies/export-jars2/changes/B1.scala b/sbt-dotty/sbt-test/source-dependencies/export-jars2/changes/B1.scala new file mode 100644 index 000000000000..586d32d544e5 --- /dev/null +++ b/sbt-dotty/sbt-test/source-dependencies/export-jars2/changes/B1.scala @@ -0,0 +1,5 @@ + +class B { + val x = new A +} + diff --git a/sbt-dotty/sbt-test/source-dependencies/export-jars2/project/DottyInjectedPlugin.scala b/sbt-dotty/sbt-test/source-dependencies/export-jars2/project/DottyInjectedPlugin.scala new file mode 100644 index 000000000000..6a77cb33121e --- /dev/null +++ b/sbt-dotty/sbt-test/source-dependencies/export-jars2/project/DottyInjectedPlugin.scala @@ -0,0 +1,12 @@ +import sbt._ +import Keys._ + +object DottyInjectedPlugin extends AutoPlugin { + override def requires = plugins.JvmPlugin + override def trigger = allRequirements + + override val projectSettings = Seq( + scalaVersion := sys.props("plugin.scalaVersion"), + scalacOptions += "-language:Scala2Compat" + ) +} diff --git a/sbt-dotty/sbt-test/source-dependencies/export-jars2/project/plugins.sbt b/sbt-dotty/sbt-test/source-dependencies/export-jars2/project/plugins.sbt new file mode 100644 index 000000000000..c17caab2d98c --- /dev/null +++ b/sbt-dotty/sbt-test/source-dependencies/export-jars2/project/plugins.sbt @@ -0,0 +1 @@ +addSbtPlugin("ch.epfl.lamp" % "sbt-dotty" % sys.props("plugin.version")) diff --git a/sbt-dotty/sbt-test/source-dependencies/export-jars2/test b/sbt-dotty/sbt-test/source-dependencies/export-jars2/test new file mode 100644 index 000000000000..925e9dee9737 --- /dev/null +++ b/sbt-dotty/sbt-test/source-dependencies/export-jars2/test @@ -0,0 +1,7 @@ +> compile +$ copy-file changes/A1.scala a/A.scala +$ copy-file changes/B1.scala b/B.scala +# This used to fail with "Tasty UUID (...) file did not correspond the tasty UUID (...) declared in the classfile" +# because we were somehow reading the .tasty file from the previous compilation run, even though it does not exist +# on disk anymore (#7852) +> compile From 9df2d1bc6cc177946e4b4aa89b6246ed5d031742 Mon Sep 17 00:00:00 2001 From: Guillaume Martres Date: Tue, 7 Jan 2020 20:09:49 +0100 Subject: [PATCH 2/2] Don't use a classloader to read .tasty files from jars The previous commit tried to fix some weird caching behaviors by closing the classloader used to read a .tasty file, but it turns out the JVM is dumb and that breaks any other in-use classloader that uses the same jar. Give up on the whole thing and work directly with the underlying ZipArchive to find the .tasty file (this just required adding a "parent" field to ZipArchive#Entry). --- .../dotc/core/classfile/ClassfileParser.scala | 44 +++++++++---------- compiler/src/dotty/tools/io/ZipArchive.scala | 27 +++++++----- 2 files changed, 37 insertions(+), 34 deletions(-) diff --git a/compiler/src/dotty/tools/dotc/core/classfile/ClassfileParser.scala b/compiler/src/dotty/tools/dotc/core/classfile/ClassfileParser.scala index 6bd476e4b650..966ccb90cbdd 100644 --- a/compiler/src/dotty/tools/dotc/core/classfile/ClassfileParser.scala +++ b/compiler/src/dotty/tools/dotc/core/classfile/ClassfileParser.scala @@ -774,16 +774,14 @@ class ClassfileParser( val attrLen = in.nextInt val bytes = in.nextBytes(attrLen) if (attrLen == 16) { // A tasty attribute with that has only a UUID (16 bytes) implies the existence of the .tasty file - val tastyBytes: Array[Byte] = classfile.underlyingSource match { // TODO: simplify when #3552 is fixed - case None => - ctx.error("Could not load TASTY from .tasty for virtual file " + classfile) - Array.empty - case Some(jar: ZipArchive) => // We are in a jar - val cl = new URLClassLoader(Array(jar.jpath.toUri.toURL), /*parent =*/ null) - try { - val path = classfile.path.stripSuffix(".class") + ".tasty" - val stream = cl.getResourceAsStream(path) - if (stream != null) { + val tastyBytes: Array[Byte] = classfile match { // TODO: simplify when #3552 is fixed + case classfile: io.ZipArchive#Entry => // We are in a jar + val path = classfile.parent.lookupName( + classfile.name.stripSuffix(".class") + ".tasty", directory = false + ) + if (path != null) { + val stream = path.input + try { val tastyOutStream = new ByteArrayOutputStream() val buffer = new Array[Byte](1024) var read = stream.read(buffer, 0, buffer.length) @@ -793,23 +791,25 @@ class ClassfileParser( } tastyOutStream.flush() tastyOutStream.toByteArray - } - else { - ctx.error(s"Could not find $path in $jar") - Array.empty + } finally { + stream.close() } } - finally { - // If we don't close the classloader, spooky things happen (see - // scripted test source-dependencies/export-jars2). - cl.close() + else { + ctx.error(s"Could not find $path in ${classfile.underlyingSource}") + Array.empty } case _ => - val plainFile = new PlainFile(io.File(classfile.jpath).changeExtension("tasty")) - if (plainFile.exists) plainFile.toByteArray - else { - ctx.error("Could not find " + plainFile) + if (classfile.jpath == null) { + ctx.error("Could not load TASTY from .tasty for virtual file " + classfile) Array.empty + } else { + val plainFile = new PlainFile(io.File(classfile.jpath).changeExtension("tasty")) + if (plainFile.exists) plainFile.toByteArray + else { + ctx.error("Could not find " + plainFile) + Array.empty + } } } if (tastyBytes.nonEmpty) { diff --git a/compiler/src/dotty/tools/io/ZipArchive.scala b/compiler/src/dotty/tools/io/ZipArchive.scala index 6058e40e9480..262e7ac66dfc 100644 --- a/compiler/src/dotty/tools/io/ZipArchive.scala +++ b/compiler/src/dotty/tools/io/ZipArchive.scala @@ -66,7 +66,7 @@ abstract class ZipArchive(override val jpath: JPath) extends AbstractFile with E def absolute: AbstractFile = unsupported() /** ''Note: This library is considered experimental and should not be used unless you know what you are doing.'' */ - sealed abstract class Entry(path: String) extends VirtualFile(baseName(path), path) { + sealed abstract class Entry(path: String, val parent: Entry) extends VirtualFile(baseName(path), path) { // have to keep this name for compat with sbt's compiler-interface def getArchive: ZipFile = null override def underlyingSource: Option[ZipArchive] = Some(self) @@ -74,7 +74,7 @@ abstract class ZipArchive(override val jpath: JPath) extends AbstractFile with E } /** ''Note: This library is considered experimental and should not be used unless you know what you are doing.'' */ - class DirEntry(path: String) extends Entry(path) { + class DirEntry(path: String, parent: Entry) extends Entry(path, parent) { val entries: mutable.HashMap[String, Entry] = mutable.HashMap() override def isDirectory: Boolean = true @@ -98,7 +98,7 @@ abstract class ZipArchive(override val jpath: JPath) extends AbstractFile with E case Some(v) => v case None => val parent = ensureDir(dirs, dirName(path), null) - val dir = new DirEntry(path) + val dir = new DirEntry(path, parent) parent.entries(baseName(path)) = dir dirs(path) = dir dir @@ -120,8 +120,9 @@ final class FileZipArchive(jpath: JPath) extends ZipArchive(jpath) { private class LazyEntry( name: String, time: Long, - size: Int - ) extends Entry(name) { + size: Int, + parent: DirEntry + ) extends Entry(name, parent) { override def lastModified: Long = time // could be stale override def input: InputStream = { val zipFile = openZipFile() @@ -140,15 +141,16 @@ final class FileZipArchive(jpath: JPath) extends ZipArchive(jpath) { // faster than LazyEntry. private class LeakyEntry( zipFile: ZipFile, - zipEntry: ZipEntry - ) extends Entry(zipEntry.getName) { + zipEntry: ZipEntry, + parent: DirEntry + ) extends Entry(zipEntry.getName, parent) { override def lastModified: Long = zipEntry.getTime override def input: InputStream = zipFile.getInputStream(zipEntry) override def sizeOption: Option[Int] = Some(zipEntry.getSize.toInt) } @volatile lazy val (root, allDirs): (DirEntry, collection.Map[String, DirEntry]) = { - val root = new DirEntry("/") + val root = new DirEntry("/", null) val dirs = mutable.HashMap[String, DirEntry]("/" -> root) val zipFile = openZipFile() val entries = zipFile.entries() @@ -163,10 +165,11 @@ final class FileZipArchive(jpath: JPath) extends ZipArchive(jpath) { new LazyEntry( zipEntry.getName(), zipEntry.getTime(), - zipEntry.getSize().toInt + zipEntry.getSize().toInt, + dir ) else - new LeakyEntry(zipFile, zipEntry) + new LeakyEntry(zipFile, zipEntry, dir) dir.entries(f.name) = f } @@ -195,7 +198,7 @@ final class FileZipArchive(jpath: JPath) extends ZipArchive(jpath) { final class ManifestResources(val url: URL) extends ZipArchive(null) { def iterator(): Iterator[AbstractFile] = { - val root = new DirEntry("/") + val root = new DirEntry("/", null) val dirs = mutable.HashMap[String, DirEntry]("/" -> root) val manifest = new Manifest(input) val iter = manifest.getEntries().keySet().iterator().asScala.filter(_.endsWith(".class")).map(new ZipEntry(_)) @@ -203,7 +206,7 @@ final class ManifestResources(val url: URL) extends ZipArchive(null) { for (zipEntry <- iter) { val dir = getDir(dirs, zipEntry) if (!zipEntry.isDirectory) { - val f = new Entry(zipEntry.getName) { + val f = new Entry(zipEntry.getName, dir) { override def lastModified = zipEntry.getTime() override def input = resourceInputStream(path) override def sizeOption = None