Skip to content

Fix race condition in local repo setup#3693

Merged
Gedochao merged 1 commit intoVirtusLab:mainfrom
unlsycn:local-repo-race
May 26, 2025
Merged

Fix race condition in local repo setup#3693
Gedochao merged 1 commit intoVirtusLab:mainfrom
unlsycn:local-repo-race

Conversation

@unlsycn
Copy link
Contributor

@unlsycn unlsycn commented May 24, 2025

Fixes #3110 and #3114 (comment).

A race condition occurs when process A acquires the lock first, creating the directory after process B's check, but before B acquires the lock while waiting for it. This problem frequently arises when running tests in parallel.

For example, consider two scala-cli process perform the following sequence of operations:

A B
Check repo dir
Acquire lock Wait for lock release
Setup repo dir Wait for lock release
Release lock
Acquire lock
Try setup → Exception

This patch fixes the issue by adding an additional post-acquisition check. Another possible fix would be to simply move the check into the withLock scope, but this would result in a redundant lock acquisition and a release each time.

Signed-off-by: unlsycn <unlsycn@unlsycn.com>
Copy link
Contributor

@Gedochao Gedochao left a comment

Choose a reason for hiding this comment

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

Looks good.
Do you have a reproduction we could test on an integration test, though?

@unlsycn
Copy link
Contributor Author

unlsycn commented May 26, 2025

Looks good.
Do you have a reproduction we could test on an integration test, though?

You know, it's hard to guarantee a stable reproduction of a concurrency bug😂. I just tested this patch on our CI machine.

But it is possible to construct a case that has a high probability of triggering this if the download takes long enough to allow another process to check the dir in this window.

@Gedochao
Copy link
Contributor

Looks good.
Do you have a reproduction we could test on an integration test, though?

You know, it's hard to guarantee a stable reproduction of a concurrency bug😂. I just tested this patch on our CI machine.

Yeah, I figured. Although was hoping you had some script you were testing it against or whatnot 😅

But it is possible to construct a case that has a high probability of triggering this if the download takes long enough to allow another process to check the dir in this window.

A follow-up with a test would be welcome, if you find the time.

Copy link
Contributor

@Gedochao Gedochao left a comment

Choose a reason for hiding this comment

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

LGTM

@Gedochao Gedochao merged commit 8049991 into VirtusLab:main May 26, 2025
54 checks passed
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.

java.nio.file.DirectoryNotEmptyException

2 participants