Don't serve stale JAR metadata from cache in classpath implementation by retronym · Pull Request #6064 · scala/scala

retronym · 2017-09-07T04:44:30Z

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

In addition to the enclosed unit test, I manually tested that
the manifestation of this problem reported in sbt/zinc#282
is solved.

% git clone sbt/zinc; cd zinc

% git co 3e9f23ae158240c4077b2d9a50cb2b5eafa8cab7~1

% sbt

> ++2.12.4-bin-a966b7d-SNAPSHOT

> compile

> # edit types.json

> compile
[error] one error found
[error] /Users/jz/code/zinc/internal/zinc-apiinfo/src/main/scala/sbt/internal/inc/ClassToAPI.scala:438: not enough arguments for constructor Projection: (x$1: xsbti.api.Type, x$2: String, x$3: String)xsbti.api.Projection.
[error] Unspecified value parameter x$3.
[error]       case None => new api.Projection(Empty, cls)
[error]                    ^

retronym · 2017-09-07T04:47:49Z

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

-    }
-    cache.getOrElseUpdate(zipFile, newClassPathInstance)
+  private def createUsingCache(zipFile: AbstractFile, settings: Settings): ClassPath = {
+    cache.getOrCreate(zipFile.file.toPath, () => createForZipFile(zipFile))


Removed needlessly noisy logging here.

lrytz · 2017-09-07T09:33:17Z

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

+  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 lastModified = Files.getLastModifiedTime(path)


it seems you can avoid calling getLastModifiedTime here (which calls Files.readAttributesinternally) and use attrs.lastModifiedTime() instead.

this could still be addressed :)

The latest commit also uses attrs.lastModifiedTime()

Sorry, my bad, I mis-read.

lrytz · 2017-09-07T09:54:07Z

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

+  def getOrCreate(path: java.nio.file.Path, create: () => T): T = cache.synchronized {
+    val lastModified = Files.getLastModifiedTime(path)
+    val attrs = Files.readAttributes(path, classOf[BasicFileAttributes])
+    val stamp = Stamp(lastModified, attrs.fileKey())


It seems attrs.fileKey is null on windows, at least on the version i checked. I guess that's fine, lastModified should be enough? Should we check still keep the fileKey check for other platforms?

Yes, on other platforms we fall back to just the timestamp check. I could declare it as Option[Object] to make it clearer that we've thought about this.

It's fine without an Option, a commend will do.

pushed a comment

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

lrytz · 2017-09-11T09:05:28Z

Very nice to have this fixed, thanks!

lrytz · 2017-09-11T09:06:49Z

At some point in the future we can hopefully remove the -YdisableFlatCpCaching setting.

retronym · 2017-09-11T23:40:57Z

At some point in the future we can hopefully remove the -YdisableFlatCpCaching setting.

Yep, that would be nice. I can think of two reasons that folks might still want to disable the caching:

With caching enabled, a long lived classloader for Global will retain strong references to JAR files, which prevents even File.finalize from closing the file descriptor. This causes problems with inability to overwrite the JAR on windows, and file descriptor leaks for use cases that use lots of different JARs over time.
In addition to the file descriptor leak, we also leak the datastructures we are caching.

So we need to have a better story for resource management here, maybe involving reference counting ("are any instance of Global actually using this right now?") and timed eviction ("when was the last time this was used?").

Those things aren't straightforward to retrofit onto a Global and Run that don't currently have a close() method, but are within the purview of scala/scala-dev#416.

@retronym

Original author is @retronym in scala/scala#6064

@retronym

Original author is @retronym in scala/scala#6064

Adapted from scalac commit bcf47b165ccfd8e1827188f70aeb24e2cecfb80f by Jason Zaugg in scala/scala#6064: 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

gkossakowski · 2017-09-29T22:03:33Z

We've hit this bug at Stripe while testing 2.12 with bazel. The incremental compilation didn't work for us. Bazel always puts all class files in a jar before passing them to a dependent target (bazel's name for something like "subproject").

Bazel makes a heavy use of symlinks and I'm wondering if cache invalidation implemented in the PR will handle them properly. I'll see whether I can get a nightly distribution build to work with bazel and verify whether 2.12's HEAD fixes this issue for us.

gkossakowski · 2017-09-29T22:41:25Z

I just verified with the latest Scala nightly: http://www.scala-lang.org/files/archive/nightly/2.12.x/scala-2.12.4-bin-c1c9c7c.tgz

The bug with under compilation in bazel goes away. This is great.

scala-jenkins added this to the 2.12.4 milestone Sep 7, 2017

retronym force-pushed the topic/cp-cache-minimal branch 2 times, most recently from a966b7d to 7b6a148 Compare September 7, 2017 04:46

retronym commented Sep 7, 2017

View reviewed changes

retronym requested a review from lrytz September 7, 2017 04:48

retronym mentioned this pull request Sep 7, 2017

Improve Classpath Implementation scala/scala-dev#416

Open

1 task

lrytz reviewed Sep 7, 2017

View reviewed changes

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

bcf47b1

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

retronym force-pushed the topic/cp-cache-minimal branch from 7b6a148 to bcf47b1 Compare September 11, 2017 01:49

lrytz approved these changes Sep 11, 2017

View reviewed changes

lrytz merged commit 4da4ede into scala:2.12.x Sep 11, 2017

allanrenucci added a commit to allanrenucci/dotty that referenced this pull request Sep 13, 2017

Port ClassPath bug fixes form scalac

e68ba95

Original author is @retronym in scala/scala#6064

allanrenucci added a commit to allanrenucci/dotty that referenced this pull request Sep 13, 2017

Port ClassPath bug fixes form scalac

5fb80fc

Original author is @retronym in scala/scala#6064

gkk-stripe mentioned this pull request Oct 12, 2017

Scala 2.12: incremental compilation broken when worker is enabled bazel-contrib/rules_scala#305

Closed

Friendseeker mentioned this pull request Oct 21, 2024

Do we need to set -YdisableFlatCpCaching for Scala 3? sbt/zinc#1480

Open

Conversation

retronym commented Sep 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lrytz commented Sep 11, 2017

Uh oh!

lrytz commented Sep 11, 2017

Uh oh!

retronym commented Sep 11, 2017

Uh oh!

gkossakowski commented Sep 29, 2017

Uh oh!

gkossakowski commented Sep 29, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

retronym commented Sep 7, 2017 •

edited

Loading