onLoad now runs with correct FileTreeRepository and CacheStoreFactory

[mkurz]

Right now when a project's onLoad gets executed, the passed State still contains the FileTreeRepository and CacheStoreFactory of a previous "lifecycle" (not sure how I should call that). For example: When running reload in the sbt console onUnload gets called, ending an "old lifecycle" and immediately afterwards onLoad for a "new lifecycle" gets executed.
The problem lies in these 3 lines:

sbt/main/src/main/scala/sbt/Main.scala

Lines 943 to 945 in 4d1af77

	val s3 = addCacheStoreFactoryFactory(Project.setProject(session, structure, s2))
	val s4 = s3.put(Keys.useLog4J.key, Project.extract(s3).get(Keys.useLog4J))
	val s5 = setupGlobalFileTreeRepository(s4)

Project.setProject(...) runs onUnload and then onLoad, however as you can see in the three lines above only afterwards both the "old" CacheStoreFactory and the FileTreeRepository get closed (when onLoad was executed already!) and new ones will be created and put in the State.

I'm pretty sure this is not correct. I would expect that the onLoad already runs with a State passed that already contains the new CacheStoreFactory and FileTreeRepository that belongs to its "lifecycle" and not those that were used in the previous, old, now un-loaded "lifecycle" and which will be closed immediately afterwards.

My proposed fix makes sure that the whole "lifecycle", from onLoad til onUnload, always uses the same FileTreeRepository and CacheStoreFactory.

Background:
We have various sbt-plugins that do some setup in onLoad, one of these plugins starts a thread and captures the State passed to onLoad and that thread later uses the captured State to execute some commands (via Commandy.process(.., capturedState)). However starting with sbt 1.4.0 we now see

java.lang.IllegalStateException: Tried to invoke register on closed repostitory sbt.internal.nio.FileTreeRepositoryImpl

after running reload in the sbt console. The weird thing is after starting sbt and the first onLoad runs everything works great, only after running reload the exception occurs.

We did test this patch by publishing sbt to our local ivy repo and I can confirm it fixes our problem. Also, this bug did not occur in sbt < 1.4.0, because it was only in 1.4 when the FileTreeRepository was closed after running onLoad (see
a2047a0#diff-cbd024cb21816130017b587e8bf7fcafc915bf438f8d8d289234b6f1c255828fR895 by @eatkins)

Thanks!

[eatkins]

I have seen this behavior myself but failed to make an issue. Thank you for not only reporting the issue but for tracking it down and proposing a fix! I made a suggestion that I think would make this a smaller change. I'm not specifically requesting that change but just would like to know if it passes your test case. If it does, then I think it would be better since it would only be a few lines instead of nearly 20. But if not, I'm happy with this as it is.

[eatkins]

LGTM.

[mkurz]

@eatkins Thank you very much for your quick review! Highly appreciated!

I updated the pull request to make use of identity, but I won't change the logic like suggested because it wouldn't be correct (see my comment)

I would like to backport this pull request to the 1.4.x branch. Should I just open another pull request for that or do you cherry-pick yourself or do you have mergify or something taking care of backports?

Thank you very much again!

[eatkins]

Yeah, you can just pull in the same changes in the 1.4.x branch and open a new pr against that branch with a PR name that starts with [1.4.x]. Also say in the description this pr number so they get linked.

[mkurz]

See #6191
Thanks again! Would love to see that fix in sbt 1.4.5 😉

[eed3si9n]

@mkurz Thanks for the contribution!

[mkurz]

You can even add this pull request to the 1.4.5 milestone now 😉