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

Avoid race condition in cleaning up cache files #45214

Merged
merged 1 commit into from
May 8, 2022

Conversation

simonbyrne
Copy link
Contributor

If multiple processes attempt to clean up cache files at the same time, a race condition can result, e.g.
https://buildkite.com/clima/climaatmos-ci/builds/812#6a961e99-e2a1-488b-a116-2a45dee26d38/102-104

If multiple processes attempt to clean up cache files at the same time, a race condition can result, e.g.
https://buildkite.com/clima/climaatmos-ci/builds/812#6a961e99-e2a1-488b-a116-2a45dee26d38/102-104
@simonbyrne simonbyrne requested a review from staticfloat May 6, 2022 23:36
@DilumAluthge
Copy link
Member

Out of curiosity, on CI, is the issue that you have two different Buildkite agents writing to the same depot at the same time? Or do you have a single Buildkite agent, and there are two different Julia processes, but they are part of the same Buildkite job?

@DilumAluthge
Copy link
Member

If it's the latter, wouldn't it be better to first do a single Pkg.precompile on a single process? Then you don't have to deal with multiple Julia processes trying to modify the compiled folder at the same time.

@simonbyrne
Copy link
Contributor Author

They're separate buildkite jobs sharing the same depot.

Copy link
Member

@DilumAluthge DilumAluthge left a comment

Choose a reason for hiding this comment

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

They're separate buildkite jobs sharing the same depot.

Ah, in that case, I don't recommend doing this PR; instead, I would recommend giving each Buildkite job its own depot. This is what we do for Base Julia CI, and it works very well. There are all kinds of issues that you can run into when multiple Buildkite agents are trying to write to the same depot at the same time.

@simonbyrne
Copy link
Contributor Author

Yeah, we do that for other cases but in this particular workflow we're trying to reduce the instantiation time.

Is there any downside of merging this PR anyway?

@simonbyrne
Copy link
Contributor Author

Note that this situation can arise in any case where multiple Julia processes are attempting to precompile at the same time, it just so happens that this particular job happens to trigger it regularly.

@giordano
Copy link
Contributor

giordano commented May 7, 2022

Ah, in that case, I don't recommend doing this PR

I think a pull request should be evaluated on its merit, not the circumstances which motivated it.

@simonbyrne simonbyrne merged commit d4acead into master May 8, 2022
@simonbyrne simonbyrne deleted the sb/compilecache-rm-force branch May 8, 2022 05:02
@simonbyrne simonbyrne added compiler:precompilation Precompilation of modules backport 1.6 Change should be backported to release-1.6 backport 1.7 backport 1.8 Change should be backported to release-1.8 labels May 8, 2022
KristofferC pushed a commit that referenced this pull request May 16, 2022
If multiple processes attempt to clean up cache files at the same time, a race condition can result, e.g.
https://buildkite.com/clima/climaatmos-ci/builds/812#6a961e99-e2a1-488b-a116-2a45dee26d38/102-104

(cherry picked from commit d4acead)
@KristofferC KristofferC mentioned this pull request May 16, 2022
45 tasks
KristofferC pushed a commit that referenced this pull request May 16, 2022
If multiple processes attempt to clean up cache files at the same time, a race condition can result, e.g.
https://buildkite.com/clima/climaatmos-ci/builds/812#6a961e99-e2a1-488b-a116-2a45dee26d38/102-104

(cherry picked from commit d4acead)
@KristofferC KristofferC mentioned this pull request May 16, 2022
67 tasks
@KristofferC KristofferC removed the backport 1.8 Change should be backported to release-1.8 label May 26, 2022
@KristofferC KristofferC removed the backport 1.6 Change should be backported to release-1.6 label Jul 6, 2022
KristofferC pushed a commit that referenced this pull request Dec 21, 2022
If multiple processes attempt to clean up cache files at the same time, a race condition can result, e.g.
https://buildkite.com/clima/climaatmos-ci/builds/812#6a961e99-e2a1-488b-a116-2a45dee26d38/102-104

(cherry picked from commit d4acead)
staticfloat pushed a commit that referenced this pull request Dec 23, 2022
If multiple processes attempt to clean up cache files at the same time, a race condition can result, e.g.
https://buildkite.com/clima/climaatmos-ci/builds/812#6a961e99-e2a1-488b-a116-2a45dee26d38/102-104

(cherry picked from commit d4acead)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler:precompilation Precompilation of modules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants