Enable lock on output dir for BSP server too #3683
Conversation
alexarchambault
force-pushed
the
out-global-lock-bsp
branch
from
113ec05
to
0dc7764
Compare
| OutLock.withLock( | ||
| noBuildLock = !outLock, | ||
| noWaitForBuildLock = false, // ??? | ||
| out = outPath, | ||
| targetsAndParams = goals.toSeq.map { | ||
| case n: NamedTask[_] => n.label | ||
| case t => t.toString | ||
| }, | ||
| streams = logger.systemStreams | ||
| ) { |
There was a problem hiding this comment.
Can we keep the locking logic out of Evaluator? The granularity of def evaluate is too fine grained, since we need to lock around not just a single evaluation but the entire MillBuildBootstrap process that may encompass multiple evaluations.
I'm actually not sure how the BSP server interacts with evaluators and the bootstrap process. Is it able to properly re-run bootstrapping if the build config changes from under it, or would it need to be explicitly restarted?
There was a problem hiding this comment.
Can we keep the locking logic out of
Evaluator?
Maybe locking could be handled by the BSP server itself. It calls evaluate here or there, so the locking could be done around those calls.
I'm actually not sure how the BSP server interacts with evaluators and the bootstrap process. Is it able to properly re-run bootstrapping if the build config changes from under it, or would it need to be explicitly restarted?
The BSP server is basically started on the side in MillMain (via the BspContext), and gets all evaluators, for meta-builds and main build, via the "mill.bsp.BSP/startSession" command (command that accepts an Evaluator.AllBootstrapEvaluators to get the evaluators).
That command is blocking, apparently to trigger reloads when clients ask for that. Maybe there's a way to make this command non-blocking, so that we can hold a lock too during the whole MillBuildBootstrap stuff that runs it…
There was a problem hiding this comment.
Got it.
In the short term, making the BSP server responsible for taking the lock whenever it calls evaluate sounds like a step forward: that is enough to ensure BSP evals do not overlap with CLI evals/bootstraps, even if it does not ensure BSP bootstraps overlap with CLI evals/bootstraps, but I assume that in any typical workflow there'll be a lot more BSP evals than there are BSP bootstraps
After that, medium term, let's see if there's some way to take the lock during the BSP bootstrap process as well, to ensure mutex between BSP bootstraps and CLI evals/bootstraps. I suspect doing it nicely may require refactoring how the BSP server is spawned and managed. I see two options:
-
Make BSP server go through
MillBuildBootstrapevery time it wants something, rather thanevaluate.- There will be some performance implications here, but maybe it can be optimized enough to be feasible.
- This would be the "most elegant" solution in ensuring the BSP and CLI go through the same code paths, and ensure full mutex
-
Make BSP server take the lock go through
MillBuildBoostrap#evaluateonly for generating theRunnerStatewhich contains the list of evaluators, then release the lock. After that, we can pass the evaluators to the BSP server, which can then take the lock as necessary when it calls evaluate on the bottom-most evaluator- This would have no performance implications over what we do today
- This would leave some race conditions in place: if BSP bootstraps, build is changed, a CLI command bootstraps, then CLI evaluates, it would evaluate using stale state and may misbehave
Of the two options, maybe try (1) and see if we can make that work? And if not we can fall back to (2)
There was a problem hiding this comment.
I could try it, but I feel 1. would be impractical. Not having go through the meta-builds is actually a nice optimization.
To work around the issue you raise in 2.ii., I would try to avoid having a BSP server run on a stale meta-build as much as possible, but also find ways for it not crash if ever it does so.
To avoid having a BSP server run on a meta-build for too long, I'd make the command spawned for BSP in MillMain ("mill.bsp.BSP/startSession") not block, so that the watch loop awakes as soon as the meta-build changes, and we can recompile the meta-build and update the evaluators ASAP. We could also prevent the BSP server from answering requests while the meta-build is being re-compiled.
To prevent rare uses of a stale meta-build to crash with NoClassDefFound and the like, I'd copy the class path of a meta-build before creating a class loader with it. In a directory specific to that evaluator. So that when other evaluators re-compile the meta-build, they don't remove class files of class loaders of other evaluators.
There was a problem hiding this comment.
To avoid having a BSP server run on a meta-build for too long, I'd make the command spawned for BSP in
MillMain("mill.bsp.BSP/startSession") not block, so that the watch loop awakes as soon as the meta-build changes, and we can recompile the meta-build and update the evaluators ASAP. We could also prevent the BSP server from answering requests while the meta-build is being re-compiled.
About that point, BSP has a notification for that, so that servers can notify clients that build targets were created / updated / deleted. Metals supports that, and Ammonite and Scala CLI notify Metals this way already.
There was a problem hiding this comment.
To prevent rare uses of a stale meta-build to crash with NoClassDefFound and the like, I'd copy the class path of a meta-build before creating a class loader with it. In a directory specific to that evaluator. So that when other evaluators re-compile the meta-build, they don't remove class files of class loaders of other evaluators.
About this one, Bloop does things along those lines already: in .bloop sub-directories, it has its own directories with compilation output, but it copies it to distinct directories for each of its clients.
There was a problem hiding this comment.
@alexarchambault what you proposed sounds reasonable, go for it
|
@alexarchambault is this ready to review/merge? |
|
@lihaoyi I guess it can be reviewed / merged. I'd like to add an integration test to ensure Mill commands and BSP requests can be interleaved. It'd be mainly useful for non-regression, to ensure upcoming changes don't break things. So it can always be added later. |
|
Sounds good, I went ahead and merged it for now |
|
Just in case, I'd manually check if BSP import in IntelliJ works fine. I didn't with the changes here. |
|
Just did, seems to work! |
No description provided.