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.

Sign up for GitHub

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

Enable lock on output dir for BSP server too #3683

Merged
merged 6 commits into from
Oct 17, 2024
Merged

Enable lock on output dir for BSP server too #3683

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
0dc7764
Enable lock on output dir for BSP server too
alexarchambault Oct 7, 2024
a8a2067
fixup
alexarchambault Oct 8, 2024
44b391d
Merge branch 'main' into pr/out-global-lock-bsp
alexarchambault Oct 10, 2024
c6e6bd1
Updates after merge
alexarchambault Oct 10, 2024
d322926
Revert everything
alexarchambault Oct 11, 2024
0bc0f17
Lock via MillBuildServer
alexarchambault Oct 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions main/eval/src/mill/eval/Evaluator.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,22 @@ trait Evaluator {
}
def disableCallgraphInvalidation: Boolean = false

/**
*/
def outLock: Boolean

/**
*/
def delayedOutLock: Boolean

/**
*/
def withOutLock(outLock: Boolean): Evaluator

/**
*/
def withDelayedOutLock(delayedOutLock: Boolean): Evaluator

@deprecated(
"Binary compatibility shim. Use overload with parameter serialCommandExec=false instead",
"Mill 0.12.0-RC1"
Expand Down
27 changes: 24 additions & 3 deletions main/eval/src/mill/eval/EvaluatorCore.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ import scala.concurrent._
private[mill] trait EvaluatorCore extends GroupEvaluator {

def baseLogger: ColorLogger
def outLock: Boolean
def delayedOutLock: Boolean

/**
* @param goals The tasks that need to be evaluated
Expand All @@ -31,8 +33,6 @@ private[mill] trait EvaluatorCore extends GroupEvaluator {
logger: ColorLogger = baseLogger,
serialCommandExec: Boolean = false
): Evaluator.Results = {
os.makeDir.all(outPath)

PathRef.validatedPaths.withValue(new PathRef.ValidatedPaths()) {
val ec =
if (effectiveThreadCount == 1) ExecutionContexts.RunNow
Expand All @@ -42,7 +42,28 @@ private[mill] trait EvaluatorCore extends GroupEvaluator {
if (effectiveThreadCount == 1) ""
else s"#${if (effectiveThreadCount > 9) f"$threadId%02d" else threadId} "

try evaluate0(goals, logger, reporter, testReporter, ec, contextLoggerMsg, serialCommandExec)
try
OutLock.withLock(
noBuildLock = !outLock,
noWaitForBuildLock = false, // ???
out = outPath,
targetsAndParams = goals.toSeq.map {
case n: NamedTask[_] => n.label
case t => t.toString
},
streams = logger.systemStreams
) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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…

Copy link
Member

@lihaoyi lihaoyi Oct 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Make BSP server go through MillBuildBootstrap every time it wants something, rather than evaluate.

    1. There will be some performance implications here, but maybe it can be optimized enough to be feasible.
    2. This would be the "most elegant" solution in ensuring the BSP and CLI go through the same code paths, and ensure full mutex

  2. Make BSP server take the lock go through MillBuildBoostrap#evaluate only for generating the RunnerState which 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

    1. This would have no performance implications over what we do today
    2. 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)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

@alexarchambault alexarchambault Oct 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

@alexarchambault alexarchambault Oct 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alexarchambault what you proposed sounds reasonable, go for it

os.makeDir.all(outPath)
evaluate0(
goals,
logger,
reporter,
testReporter,
ec,
contextLoggerMsg,
serialCommandExec
)
}
finally ec.close()
}
}
Expand Down
7 changes: 7 additions & 0 deletions main/eval/src/mill/eval/EvaluatorImpl.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ private[mill] case class EvaluatorImpl(
home: os.Path,
workspace: os.Path,
outPath: os.Path,
outLock: Boolean,
delayedOutLock: Boolean,
externalOutPath: os.Path,
override val rootModule: mill.define.BaseModule,
baseLogger: ColorLogger,
Expand All @@ -43,6 +45,11 @@ private[mill] case class EvaluatorImpl(
override def withFailFast(newFailFast: Boolean): Evaluator =
this.copy(failFast = newFailFast)

override def withOutLock(outLock: Boolean): Evaluator =
copy(outLock = outLock)
override def withDelayedOutLock(delayedOutLock: Boolean): Evaluator =
copy(delayedOutLock = delayedOutLock)

override def plan(goals: Agg[Task[_]]): (MultiBiMap[Terminal, Task[_]], Agg[Task[_]]) = {
Plan.plan(goals)
}
Expand Down
50 changes: 50 additions & 0 deletions main/eval/src/mill/eval/OutLock.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
package mill.eval

import mill.api.SystemStreams
import mill.main.client.OutFiles
import mill.main.client.lock.Lock

import scala.util.Using

object OutLock {

def withLock[T](
noBuildLock: Boolean,
noWaitForBuildLock: Boolean,
out: os.Path,
targetsAndParams: Seq[String],
streams: SystemStreams
)(t: => T): T = {
if (noBuildLock) t
else {
val outLock = Lock.file((out / OutFiles.millLock).toString)

def activeTaskString =
try {
os.read(out / OutFiles.millActiveCommand)
} catch {
case e => "<unknown>"
}

def activeTaskPrefix = s"Another Mill process is running '$activeTaskString',"
Using.resource {
val tryLocked = outLock.tryLock()
if (tryLocked.isLocked()) tryLocked
else if (noWaitForBuildLock) {
throw new Exception(s"$activeTaskPrefix failing")
} else {

streams.err.println(
s"$activeTaskPrefix waiting for it to be done..."
)
outLock.lock()
}
} { _ =>
os.write.over(out / OutFiles.millActiveCommand, targetsAndParams.mkString(" "))
try t
finally os.remove.all(out / OutFiles.millActiveCommand)
}
}
}

}
21 changes: 13 additions & 8 deletions main/src/mill/main/RunScript.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,14 +20,19 @@ object RunScript {
String,
(Seq[Watchable], Either[String, Seq[(Any, Option[(TaskName, ujson.Value)])]])
] = {
val resolved = mill.eval.Evaluator.currentEvaluator.withValue(evaluator) {
Resolve.Tasks.resolve(
evaluator.rootModule,
scriptArgs,
selectMode,
evaluator.allowPositionalCommandArgs
)
}
val resolved =
mill.eval.Evaluator.currentEvaluator.withValue(
evaluator
.withOutLock(evaluator.delayedOutLock)
.withDelayedOutLock(false)
) {
Resolve.Tasks.resolve(
evaluator.rootModule,
scriptArgs,
selectMode,
evaluator.allowPositionalCommandArgs
)
}
for (targets <- resolved) yield evaluateNamed(evaluator, Agg.from(targets))
}

Expand Down
11 changes: 10 additions & 1 deletion runner/src/mill/runner/MillBuildBootstrap.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ import java.net.URLClassLoader
class MillBuildBootstrap(
projectRoot: os.Path,
output: os.Path,
delayedOutLock: Boolean,
home: os.Path,
keepGoing: Boolean,
imports: Seq[String],
Expand Down Expand Up @@ -309,7 +310,13 @@ class MillBuildBootstrap(
assert(nestedState.frames.forall(_.evaluator.isDefined))

val (evaled, evalWatched, moduleWatches) = Evaluator.allBootstrapEvaluators.withValue(
Evaluator.AllBootstrapEvaluators(Seq(evaluator) ++ nestedState.frames.flatMap(_.evaluator))
Evaluator.AllBootstrapEvaluators(
(Seq(evaluator) ++ nestedState.frames.flatMap(_.evaluator)).map { ev =>
ev
.withOutLock(ev.delayedOutLock)
.withDelayedOutLock(false)
}
)
) {
evaluateWithWatches(rootModule, evaluator, targetsAndParams)
}
Expand Down Expand Up @@ -345,6 +352,8 @@ class MillBuildBootstrap(
home,
projectRoot,
recOut(output, depth),
false,
delayedOutLock,
recOut(output, depth),
rootModule,
new PrefixLogger(logger, bootLogPrefix),
Expand Down
48 changes: 6 additions & 42 deletions runner/src/mill/runner/MillMain.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import scala.util.Properties
import mill.java9rtexport.Export
import mill.api.{MillException, SystemStreams, WorkspaceRoot, internal}
import mill.bsp.{BspContext, BspServerResult}
import mill.eval.OutLock
import mill.main.BuildInfo
import mill.main.client.{OutFiles, ServerFiles}
import mill.main.client.lock.Lock
Expand Down Expand Up @@ -213,6 +214,8 @@ object MillMain {

val out = os.Path(OutFiles.out, WorkspaceRoot.workspaceRoot)

val delayLock = bspContext.isDefined

var repeatForBsp = true
var loopRes: (Boolean, RunnerState) = (false, RunnerState.empty)
while (repeatForBsp) {
Expand All @@ -226,8 +229,8 @@ object MillMain {
evaluate = (prevState: Option[RunnerState]) => {
adjustJvmProperties(userSpecifiedProperties, initialSystemProperties)

withOutLock(
config.noBuildLock.value || bspContext.isDefined,
OutLock.withLock(
config.noBuildLock.value || delayLock,
config.noWaitForBuildLock.value,
out,
targetsAndParams,
Expand All @@ -250,6 +253,7 @@ object MillMain {
try new MillBuildBootstrap(
projectRoot = WorkspaceRoot.workspaceRoot,
output = out,
delayedOutLock = delayLock,
home = config.home,
keepGoing = config.keepGoing.value,
imports = config.imports,
Expand Down Expand Up @@ -416,44 +420,4 @@ object MillMain {
for (k <- systemPropertiesToUnset) System.clearProperty(k)
for ((k, v) <- desiredProps) System.setProperty(k, v)
}

def withOutLock[T](
noBuildLock: Boolean,
noWaitForBuildLock: Boolean,
out: os.Path,
targetsAndParams: Seq[String],
streams: SystemStreams
)(t: => T): T = {
if (noBuildLock) t
else {
val outLock = Lock.file((out / OutFiles.millLock).toString)

def activeTaskString =
try {
os.read(out / OutFiles.millActiveCommand)
} catch {
case e => "<unknown>"
}

def activeTaskPrefix = s"Another Mill process is running '$activeTaskString',"
Using.resource {
val tryLocked = outLock.tryLock()
if (tryLocked.isLocked()) tryLocked
else if (noWaitForBuildLock) {
throw new Exception(s"$activeTaskPrefix failing")
} else {

streams.err.println(
s"$activeTaskPrefix waiting for it to be done..."
)
outLock.lock()
}
} { _ =>
os.write.over(out / OutFiles.millActiveCommand, targetsAndParams.mkString(" "))
try t
finally os.remove.all(out / OutFiles.millActiveCommand)
}
}
}

}
2 changes: 2 additions & 0 deletions testkit/src/mill/testkit/UnitTester.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,8 @@ class UnitTester(
mill.api.Ctx.defaultHome,
module.millSourcePath,
outPath,
false,
false,
outPath,
module,
logger,
Expand Down
Loading