Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Be more resilient when reading previous code hash signatures #3849

Closed
wants to merge 4 commits into from

Conversation

lefou
Copy link
Member

@lefou lefou commented Oct 26, 2024

I saw exceptions originating here when I changed between Mill versions. I think, this happen, when we made an incompatible change. Since this is no real breakage, but only some dropping of optimizations, I think we better recover without interruption than breaking in front of the user without any guidance (e.g. removing out/mill-build) how to solve.

Now, we just ignore the previous state.

Here's the stack trace:

> mill __.compile
Downloading mill 0.12.0 from https://repo1.maven.org/maven2/com/lihaoyi/mill-dist/0.12.0/mill-dist-0.12.0.jar ...
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 64.6M  100 64.6M    0     0   676k      0  0:01:37  0:01:37 --:--:--  761k
[61/61] =============================================================================================================== __.compile ================================================================================================================= 3s
1 tasks failed
methodCodeHashSignatures os.ResourceNotFoundException: java.net.URLClassLoader@70691692/mill/runner/MillBuildRootModule$MillMiscInfo.class
    os.ResourcePath.getInputStream(ResourcePath.scala:21)
    os.read$inputStream$.apply(ReadWriteOps.scala:245)
    mill.codesig.ExternalSummary$.load0$1(ExternalSummary.scala:55)
    mill.codesig.ExternalSummary$.$anonfun$apply$6(ExternalSummary.scala:48)
    scala.runtime.java8.JFunction0$mcV$sp.apply(JFunction0$mcV$sp.scala:18)
    scala.collection.mutable.HashMap.getOrElse(HashMap.scala:451)
    mill.codesig.ExternalSummary$.load$1(ExternalSummary.scala:48)
    mill.codesig.ExternalSummary$.$anonfun$apply$9(ExternalSummary.scala:69)
    mill.codesig.ExternalSummary$.$anonfun$apply$9$adapted(ExternalSummary.scala:69)
    scala.collection.immutable.BitmapIndexedSetNode.foreach(HashSet.scala:951)
    scala.collection.immutable.HashSet.foreach(HashSet.scala:958)
    mill.codesig.ExternalSummary$.apply(ExternalSummary.scala:69)
    mill.codesig.CodeSig$.compute(CodeSig.scala:17)
    mill.runner.MillBuildRootModule.$anonfun$methodCodeHashSignatures$2(MillBuildRootModule.scala:173)

@lefou lefou force-pushed the resilient-code-hash-read branch from af9f8c2 to fb9f233 Compare October 27, 2024 21:45
@lefou lefou marked this pull request as ready for review October 28, 2024 09:26
@lefou lefou requested a review from lihaoyi October 28, 2024 09:26
@lihaoyi
Copy link
Member

lihaoyi commented Oct 28, 2024

@lefou do you have the error message to include in the PR description?

@lefou
Copy link
Member Author

lefou commented Oct 28, 2024

@lefou do you have the error message to include in the PR description?

I updated the description.

@lihaoyi
Copy link
Member

lihaoyi commented Oct 28, 2024

Seems like the change in the PR isn't in the right place. According to the stack trace, the problematic read is happening here

new ClassReader(os.read.inputStream(resourcePath)).accept(
visitor,
ClassReader.SKIP_CODE | ClassReader.SKIP_FRAMES | ClassReader.SKIP_DEBUG
)

@lihaoyi
Copy link
Member

lihaoyi commented Oct 28, 2024

Do you have a repro for this? It's probably worth understanding properly what's going on since it might indicate a deeper misbehavior

@lefou
Copy link
Member Author

lefou commented Oct 28, 2024

TBH, I saw this exception multiple times in the last days but have no idea how to reproduce except that I know I switched between Mill versions (all within the 0.12.0 line, only PRs or some commits forward or backward). So I can't tell what exactly is causing the issue.

@lefou
Copy link
Member Author

lefou commented Oct 28, 2024

My suspicion is, that we don't properly track version change and therefore don't invalidate the meta-build correctly.

@lihaoyi
Copy link
Member

lihaoyi commented Oct 28, 2024

From a quick skim, it seems the code does the following:

  1. It scans compile().classes.path to find all locally compiled classfiles
  2. It looks for all "external" classes that appear in method signatures and super-types among those local classfiles, but are not in the local set
  3. It tries to classload those external classes from the compileClasspath().toSeq.map(_.path)

So given the stack trace, it looks like mill/runner/MillBuildRootModule$MillMiscInfo.class must be a supertype or method signature type of some locally compiled class, while not existing in the compileClasspath(). I'm not sure how that can happen, but it must be happening somehow for this error to appear

One thing worth calling out is that MillBuildRootModule.MillMiscInfo existed in Mill 0.11.x but does not exist in Mill 0.12.x, and so somehow when you changed branch, the compile().classes.path contains a reference to the old class even though the compileClasspath() that compile().classes.path was compiled against no longer has that class at all

@lefou
Copy link
Member Author

lefou commented Oct 28, 2024

Could be related to the use of Mill plugins which are mostly compiled against Mill 0.11.0?

@lefou lefou marked this pull request as draft October 28, 2024 11:34
@lihaoyi
Copy link
Member

lihaoyi commented Oct 28, 2024

@lefou seems unlikely, the reference must be coming from something compiled locally. But maybe there's some pathway that I'm missing for the information to slip through.

Next time this happens, if you could zip up your out/mill-build/compile.dest/ folder and send it to me, I can poke through and see where this reference is coming from. You can also do it yourself if you wish: just loop over all the classfiles with javap -c | grep MillBuildRootModule$MillMiscInfo and hopefully it will tell us who is referencing the problematic class, and from there we can figure out how it's getting there

@lefou
Copy link
Member Author

lefou commented Oct 28, 2024

It can be, that I also switched from a branch which is still on Mill 0.11.12. I can try to reproduce and minimize it.

Beside that reproducer, finding some older run state under out/ should not cause any Mill errors. We must miss the fact that we changed the Mill version. Could be the use of this non-content based PathRef hash?

val millBootClasspathPathRefs: Seq[PathRef] = millBootClasspath.map(PathRef(_, quick = true))

@lefou
Copy link
Member Author

lefou commented Oct 30, 2024

I ran into another instance of a ResourceNotFoundException, but this time the resources does not originate in Mill. I'll open a new issue...

@lihaoyi
Copy link
Member

lihaoyi commented Oct 30, 2024

Interesting. This suggests that the issue isn't specifically related to changing Mill version, or the Mill assembly classpath, but is a more general issue with changes in the mill-build classpath including locally-compiled classes

@lefou
Copy link
Member Author

lefou commented Oct 30, 2024

Yeah. Could be some wrong assumption in the logic but also something external like incremental under-compilation.

@lefou
Copy link
Member Author

lefou commented Nov 1, 2024

Superseded by #3884

@lefou lefou closed this Nov 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants