Fix possible race condition around file locks #5152
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
`FileLock#lock` doesn't always block until it can acquire a lock. If the lock is held by the same JVM, it just throws an `OverlappingFileLockException`. To work around that, this replaces a call to `FileLock#lock` by a while loop that calls `FileLock#tryLock`, that stops when the lock is actually acquired.
|
Before this PR, |
|
Maybe we could just make |
|
If the problem is locks within the same JVM, this use case is meant to be handled by |
|
When making this test more verbose, I'm getting this stack trace at some point |
|
Seems like we only rolled out |
|
One question I'd be curious though, is why would |
|
Doing this, but that doesn't fix the issue diff --git a/runner/daemon/src/mill/daemon/MillMain.scala b/runner/daemon/src/mill/daemon/MillMain.scala
index 25887c2ca6d..f795412fae3 100644
--- a/runner/daemon/src/mill/daemon/MillMain.scala
+++ b/runner/daemon/src/mill/daemon/MillMain.scala
@@ -3,7 +3,7 @@ package mill.daemon
import mill.api.internal.{BspServerResult, internal}
import mill.api.{Logger, MillException, Result, SystemStreams}
import mill.bsp.BSP
-import mill.client.lock.Lock
+import mill.client.lock.{DoubleLock, Lock}
import mill.constants.{DaemonFiles, OutFiles, Util}
import mill.define.BuildCtx
import mill.internal.{Colors, MultiStream, PromptLogger}
@@ -58,6 +58,10 @@ object MillMain {
}
)
+ val outLock = new DoubleLock(
+ Lock.memory(),
+ Lock.file((out / OutFiles.millOutLock).toString)
+ )
val daemonDir = os.Path(args.head)
val (result, _) =
try main0(
@@ -71,7 +75,7 @@ object MillMain {
initialSystemProperties = sys.props.toMap,
systemExit = i => sys.exit(i),
daemonDir = daemonDir,
- outLock = Lock.file((out / OutFiles.millOutLock).toString)
+ outLock = outLock
)
catch handleMillException(initialSystemStreams.err, ()) |
|
The full stack trace I get when making the test more verbose is |
|
But I should use the same memory lock instance, maybe |
|
Perhaps the problem is that this lock should also be a double lock? |
|
is the problem reproducible? It would be worth adding some logging and seeing exactly what scenario is happening that is causing the lock to be taken twice |
|
It's reproducible locally for sure, I seem to always run into it. Don't know yet about the CI, it happened once here. |
|
It seems the problem is that |
|
Why do we call |
|
Seems permuting these two lines fixes it |
|
Closing in favor of #5153 |
alexarchambault
added a commit
that referenced
this pull request
May 19, 2025
This PR makes some uses of `DoubleLock` safer, and fixes a race
condition with it.
`DoubleLock` is basically used as a wrapper around a file lock (second
lock), using it alongside a memory lock (first lock), that acts as a
mutex for it. That way, the same JVM process doesn't try to acquire the
file lock twice at the same time in a given JVM, which
`java.nio.channels.FileChannel` doesn't allow (it throws an
`OverlappingFileLockException` in that case).
This PR makes sure `MillMain` and `MillDaemonMain` use the same
`Lock.memory()` instance as a guard when trying to acquire a lock for
the output directory. This doesn't solve the issue I was running into
(see below), but seems to be safer nonetheless.
It also tries to dispose of locks in `DoubleLock` in case calls to
`Lock#{lock,tryLock}` throw.
Lastly, it releases the two locks in `DoubleLocked` in the reverse order
that they were acquired. This fixes the test case below. Doing so fixes
a race condition, where
- a first thread acquires the lock
- a second one tries to acquire it, and waits for it
- the first thread releases the lock
During the third step, releasing the memory lock before the file lock
makes the second thread acquire the memory lock, and then possibly try
to acquire the file lock, before the first thread is done releasing the
file lock. The JVM doesn't allow this, and the second thread gets an
`OverlappingFileLockException`.
See the discussion in #5152 too
This fixes
```text
$ ./mill 'integration.feature[output-directory].packaged.daemon.testForked' "mill.integration.OutputDirectoryLockTests.basic"
```
for me, locally.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
FileLock#lockdoesn't always block until it can acquire a lock. If the lock is held by the same JVM, it just throws anOverlappingFileLockException. To work around that, this replaces a call toFileLock#lockby a while loop that callsFileLock#tryLock, that stops when the lock is actually acquired.