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

Acquire a lock on the out dir in order to run tasks / commands #3599

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

alexarchambault
Copy link
Contributor

First cut at #3519

There's no tests yet, but it works fine in manual tests.

@alexarchambault
Copy link
Contributor Author

alexarchambault commented Sep 23, 2024

This results in things like

mill-global-lock.mov

We see the lock in action when the 3 sessions start (only one compiles the build while the other wait for the lock), and when they run the long-running command, with only one session running the command at a time.

@alexarchambault
Copy link
Contributor Author

Note that the lock is only for write operations, not those that just read things. In my experience, issues happen mainly when workers intend to run tasks and write things at the same time. But a shared lock (allowing several workers to hold it while they read things) could be added too.

@alexarchambault
Copy link
Contributor Author

(This includes #3530)

@lihaoyi
Copy link
Member

lihaoyi commented Sep 24, 2024

For a first pass, let's place the lock around the entire MillMain.main0 workflow. It might be more coarse grained than necessary, but I think it's better to go for correctness first and we can figure out the finer-grained concurrency story later.

As an escape hatch, we should add a flag --no-build-lock or something so people who depend on concurrent Mill workflows can continue to do so (at their own risk, but not any more risk than they have today)

Lastly, let's make the Waiting for lock... message a bit more informative. Someone hitting Waiting for lock... probably has Mill running with -w in some other terminal they forgot about, or their IDE is running Mill on their behalf, and they would want to be told what is going on so they know what to do (use their other terminal, cancel Mill in their other terminal, wait for their IDE to finish indexing or cancel it). We could store the Mill command line args in an out/mill-current-command.txt file or something and display it as part of the message e.g. Waiting for running Mill command ./mill -w __.compile to finish... so they have some idea of what is going on and what they can do about it

@alexarchambault
Copy link
Contributor Author

Isn't a lock around the whole of MillMain.main0 too much? That would basically ensure only one Mill process is running at a time on a project, IIUC. It would definitely break many of my workflows (I have apps and tests poking with the terminal in many of my projects, so I tend to use -i very often, and often run several Mill commands at the same time in different terminals - plus a BSP server). A lock on MillMain.main0 would be quite harmful in those scenarios.

@alexarchambault
Copy link
Contributor Author

The last push makes the lock slightly broader compared to the previous state of this PR. The lock was around two things: EvaluatorCore.evaluate0, and the writing of the mill-runner-state.json file (look for the logger.waitForLock calls in the PR), it's now only around MillBuildBootstrap.evaluate.

Note that BSP is currently broken when the lock is enabled: the BSP server runs via the mill.bsp.BSP/startSession command, that itself runs other tasks / commands, and the lock isn't reentrant, so that the latter fails.

@alexarchambault alexarchambault force-pushed the out-global-lock branch 2 times, most recently from 19ccd1f to aa2c64a Compare September 24, 2024 22:17
@alexarchambault alexarchambault changed the title Don't run tasks concurrently, via a global lock Acquire a lock on the out dir in order to run tasks / commands Sep 24, 2024
@alexarchambault alexarchambault marked this pull request as draft September 24, 2024 22:18
@lihaoyi
Copy link
Member

lihaoyi commented Sep 24, 2024

If I understand correctly, putting the lock around MillBuildBootstrap#evaluate rather than MillMain.main0 would allow the lock to be released when -w is watching for files but not actually executing anything. I think that sounds great.

For BSP support, how about we disable the lock for BSP entirely, with the expectation that once #3530 lands we can move the BSP code path to use .bsp/out/ as its work folder which should eliminate conflicts between BSP and non-BSP commands. That would end up with manual CLI commands single-threaded due to locking on out/, and BSP commands single-threaded due to running isolated in .bsp/out/, at the cost of some duplication of work (hopefully not too much, since typically BSP does not need to compile the actual Scala code in the common case)

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