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

add a suffix to a new cache files in case of failure of renaming it to an exisiting cache file already in use #48137

Merged
merged 3 commits into from
Jan 9, 2023

Conversation

KristofferC
Copy link
Member

@KristofferC KristofferC commented Jan 5, 2023

In cases where we try to compile a cache file for a package that is already in use in another Julia session, Windows gives us an error because we are trying to rename a file to another file that is already in use (#48074).

This PR detects when the rename fails and adds a suffix _i to the cache file name where i is the smallest integer such that that file does not exist.

Starting a few julia sessions and loading Example while editing it between every load gives a list of files like:

PS C:\Users\Kristoffer\.julia\compiled\v1.10\Example> dir

    Directory: C:\Users\Kristoffer\.julia\compiled\v1.10\Example
Mode                 LastWriteTime         Length Name
----                 -------------         ------ ----
-a---          2023-01-05    14:11          20480 lLvWP_7lexV_1.dll
-a---          2023-01-05    14:11           2732 lLvWP_7lexV_1.ji
-a---          2023-01-05    14:12          20480 lLvWP_7lexV_2.dll
-a---          2023-01-05    14:12           2738 lLvWP_7lexV_2.ji
-a---          2023-01-05    14:11          20480 lLvWP_7lexV.dll
-a---          2023-01-05    14:11           2728 lLvWP_7lexV.ji

Fixes #48074

@KristofferC KristofferC added system:windows Affects only Windows packages Package management and loading labels Jan 5, 2023
@gbaraldi
Copy link
Member

gbaraldi commented Jan 5, 2023

Why are we trying to overwrite the cache files? If they aren't different shouldn't they have different hashes?

@KristofferC
Copy link
Member Author

There is a maximum of 10 cache files for a package before we start overwriting old files. If you are continuously working on a package that means you would quickly start deleting cache files used by other projects. In the example above, I would have deleted the cache file for Example that I use in my global project. So that is why a cache file for a given project will overwrite an earlier cachefile from the same project.

But there might be better strategies and heuristics.

@vchuravy vchuravy requested a review from vtjnash January 5, 2023 15:19
Copy link
Member

@vchuravy vchuravy left a comment

Choose a reason for hiding this comment

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

LGTM, but I would love to have the POSIX semantics that C++ now mandates in libuv ;)

base/loading.jl Outdated
Comment on lines 2118 to 2132
old_cachefiles = filter(x->startswith(x, basename(ocachename)) && endswith(x, ocacheext), readdir(cachepath))
nums = Set(1:length(old_cachefiles))
for file in old_cachefiles
name = splitext(file)[1]
s = split(name, '_')
if length(s) == 3 # e.g. lLvWP_g5TNZ_3
i = tryparse(Int, last(s))
if i !== nothing
delete!(nums, i)
end
end
end
# TODO: Risk for a race here if some other process grabs this name before us
num = isempty(nums) ? 1 : minimum(nums)
ocachefile = ocachename * "_$num" * ocacheext
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
old_cachefiles = filter(x->startswith(x, basename(ocachename)) && endswith(x, ocacheext), readdir(cachepath))
nums = Set(1:length(old_cachefiles))
for file in old_cachefiles
name = splitext(file)[1]
s = split(name, '_')
if length(s) == 3 # e.g. lLvWP_g5TNZ_3
i = tryparse(Int, last(s))
if i !== nothing
delete!(nums, i)
end
end
end
# TODO: Risk for a race here if some other process grabs this name before us
num = isempty(nums) ? 1 : minimum(nums)
ocachefile = ocachename * "_$num" * ocacheext
old_cachefiles = Set(readdir(cachepath))
num = 1
while true
ocachefile = ocachename * "_$num" * ocacheext
# ispath(ocachefile) || break
in(ocachefile, old_cachefiles) || break
end
# TODO: Risk for a race here if some other process grabs this name before us

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, this part felt a bit convoluted.

Copy link
Member

Choose a reason for hiding this comment

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

We could also try to use tempname, since it implements mostly the same logic as this, but possibly does not support suffixes

@DilumAluthge
Copy link
Member

Backport to 1.9?

@KristofferC KristofferC added the backport 1.9 Change should be backported to release-1.9 label Jan 5, 2023
@grandinj
Copy link

grandinj commented Jan 7, 2023

Note that Win32 and Linux have different semantics and API around atomic filesystem operations.

Ideally you want to be using the ReplaceFile function on windows.

@vtjnash
Copy link
Member

vtjnash commented Jan 7, 2023

You don't though, since that function is pretty weird and not like posix nor atomic. You want MoveFile, which is what this calls.

@KristofferC KristofferC merged commit ec437b7 into master Jan 9, 2023
@KristofferC KristofferC deleted the kc/win_xd branch January 9, 2023 18:44
KristofferC added a commit that referenced this pull request Jan 10, 2023
…o an exisiting cache file already in use (#48137)

* add a suffix to a new cache files in case of failure of renaming it to an exisiting file

(cherry picked from commit ec437b7)
@KristofferC KristofferC mentioned this pull request Jan 10, 2023
41 tasks
@KristofferC KristofferC removed the backport 1.9 Change should be backported to release-1.9 label Jan 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
packages Package management and loading system:windows Affects only Windows
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Precompiled dll unlinking permission denied error on Windows 1.9.0-beta2
6 participants