Change delayed delete mechanism to prevent cross-drive mv failure for in-use DLL (#59635)

On Windows, during precompilation of package `A`, a DLL file is
generated and may replace the existing one. If `A` is already loaded in
the julia session however, the corresponding soon-to-be-obsolete DLL
cannot be removed while julia is running. Currently, this problem is
solved by moving the old DLL in a special "julia_delayed_deletes"
folder, which will be cleaned in a later julia session by `Pkg.gc()`.
However, moving an in-use DLL is only permitted on the same drive, and
the "julia_delayed_deletes" is located in the `tempdir`, a.k.a. on a
fixed drive, say `C:`. Thus, if the `DEPOT_PATH` points to a ".julia" in
another drive, say `D:`, any precompilation occuring on an already
loaded package will fail. This is what happens in #59589, actually
resulting in an infinite loop that bloats up both memory and disk.
@staticfloat had actually predicted that such an issue could occur in
#53456 (comment).

This PR fixes #59589 by changing the "delayed deleting" mechanism:
instead of moving the old DLLs to a fixed folder, they are renamed in
their initial folder and recorded in a list stored at a fixed location.
Upon `Pkg.gc()`, the listed obsolete files will be deleted
(JuliaLang/Pkg.jl#4392).

It also prevents any similar infinite loops by cutting the `rm -> mv ->
rm` cycle into `rm -> rename`. ~I also removed the private argument
`allow_delayed_delete` from `rm`, which is only called in by
[Pkg](https://github.com/JuliaLang/Pkg.jl/blob/7344e2656475261a83a6bd37d9d4cc1e7dcf5f0d/src/API.jl#L1127)
but does not appear to be useful.~

EDIT: current state is
#59635 (comment)

---------

Co-authored-by: Jameson Nash <[email protected]>
Co-authored-by: Elliot Saba <[email protected]>

Author: 3 people

Compiler/test/special_loading.jl

@@ -2,7 +2,8 @@
 
 # Only run when testing Base compiler
 if Base.identify_package("Compiler") === nothing
-    mktempdir() do dir
+    include(joinpath(Sys.BINDIR, Base.DATAROOTDIR, "julia", "test", "tempdepot.jl"))
+    mkdepottempdir() do dir
         withenv("JULIA_DEPOT_PATH" => dir * (Sys.iswindows() ? ";" : ":"), "JULIA_LOAD_PATH" => nothing) do
             cd(joinpath(@__DIR__, "CompilerLoadingTest")) do
                 @test success(pipeline(`$(Base.julia_cmd()[1]) --startup-file=no --project=. compiler_loading_test.jl`; stdout, stderr))

base/file.jl

@@ -253,9 +253,9 @@ function mkpath(path::AbstractString; mode::Integer = 0o777)
     return path
 end
 
-# Files that were requested to be deleted but can't be by the current process
-# i.e. loaded DLLs on Windows
-delayed_delete_dir() = joinpath(tempdir(), "julia_delayed_deletes")
+# Files that were requested to be deleted but can't be by the current process,
+# i.e. loaded DLLs on Windows, are listed in the directory below
+delayed_delete_ref() = joinpath(tempdir(), "julia_delayed_deletes_ref")
 
 """
     rm(path::AbstractString; force::Bool=false, recursive::Bool=false)
@@ -288,13 +288,7 @@ function rm(path::AbstractString; force::Bool=false, recursive::Bool=false, allo
                 force && err.code==Base.UV_ENOENT && return
                 @static if Sys.iswindows()
                     if allow_delayed_delete && err.code==Base.UV_EACCES && endswith(path, ".dll")
-                        # Loaded DLLs cannot be deleted on Windows, even with posix delete mode
-                        # but they can be moved. So move out to allow the dir to be deleted.
-                        # Pkg.gc() cleans up this dir when possible
-                        dir = mkpath(delayed_delete_dir())
-                        temp_path = tempname(dir, cleanup = false, suffix = string("_", basename(path)))
-                        @debug "Could not delete DLL most likely because it is loaded, moving to tempdir" path temp_path
-                        mv(path, temp_path)
+                        delayed_delete_dll(path)
                         return
                     end
                 end
@@ -330,6 +324,22 @@ function rm(path::AbstractString; force::Bool=false, recursive::Bool=false, allo
 end
 
 
+# Loaded DLLs cannot be deleted on Windows, even with posix delete mode but they can be renamed.
+# delayed_delete_dll(path) does so temporarily, until later cleanup by Pkg.gc().
+function delayed_delete_dll(path)
+    # in-use DLL must be kept on the same drive
+    temp_path = tempname(abspath(dirname(path)); cleanup=false, suffix=string("_", basename(path)))
+    @debug "Could not delete DLL most likely because it is loaded, moving to a temporary path" path temp_path
+    mkpath(delayed_delete_ref())
+    io = last(mktemp(delayed_delete_ref(); cleanup=false))
+    try
+        print(io, temp_path) # record the temporary path for Pkg.gc()
+    finally
+        close(io)
+    end
+    rename(path, temp_path) # do not call mv which could recursively call rm(path)
+end
+
 # The following use Unix command line facilities
 function checkfor_mv_cp_cptree(src::AbstractString, dst::AbstractString, txt::AbstractString;
                                                           force::Bool=false)
@@ -678,8 +688,35 @@ end
 # deprecated internal function used by some packages
 temp_cleanup_purge(; force=false) = force ? temp_cleanup_purge_all() : @lock TEMP_CLEANUP_LOCK temp_cleanup_purge_prelocked(false)
 
+function temp_cleanup_postprocess(cleanup_dirs)
+    if !isempty(cleanup_dirs)
+        rmcmd = """
+        cleanuplist = readlines(stdin) # This loop won't start running until stdin is closed, which is supposed to be sequenced after the process exits
+        sleep(1) # Wait for the operating system to hopefully be ready, since the OS implementation is probably incorrect, given the history of buggy work-arounds like this that have existed for ages in dotNet and libuv
+        for path in cleanuplist
+            try
+                rm(path, force=true, recursive=true)
+            catch ex
+                @warn "Failed to clean up temporary path \$(repr(path))\n\$ex" _group=:file
+            end
+        end
+        """
+        cmd = Cmd(Base.cmd_gen(((Base.julia_cmd(),), ("--startup-file=no",), ("-e",), (rmcmd,))); ignorestatus = true, detach = true)
+        pw = Base.PipeEndpoint()
+        run(cmd, pw, devnull, stderr; wait=false)
+        join(pw, cleanup_dirs, "\n")
+        Base.dup(Base._fd(pw)) # intentionally leak a reference, until the process exits
+        close(pw)
+    end
+end
+
+function temp_cleanup_atexit()
+    temp_cleanup_purge_all()
+    @lock TEMP_CLEANUP_LOCK temp_cleanup_postprocess(keys(TEMP_CLEANUP))
+end
+
 function __postinit__()
-    Base.atexit(temp_cleanup_purge_all)
+    Base.atexit(temp_cleanup_atexit)
 end
 
 const temp_prefix = "jl_"

test/compileall.jl

@@ -2,8 +2,10 @@
 # We make it a separate test target here, so that it can run in parallel
 # with the rest of the tests.
 
+include("tempdepot.jl")
+
 function precompile_test_harness(@nospecialize(f))
-    load_path = mktempdir()
+    load_path = mkdepottempdir()
     try
         pushfirst!(LOAD_PATH, load_path)
         pushfirst!(DEPOT_PATH, load_path)

test/core.jl

@@ -9,6 +9,8 @@ const Bottom = Union{}
 # For curmod_*
 include("testenv.jl")
 
+include("tempdepot.jl")
+
 ## tests that `const` field declarations
 
 # sanity tests that our built-in types are marked correctly for const fields
@@ -8443,7 +8445,7 @@ end
 
 # precompilation
 let load_path = mktempdir()
-    depot_path = mktempdir()
+    depot_path = mkdepottempdir()
     try
         pushfirst!(LOAD_PATH, load_path)
         pushfirst!(DEPOT_PATH, depot_path)
@@ -8485,11 +8487,6 @@ let load_path = mktempdir()
         filter!((≠)(load_path), LOAD_PATH)
         filter!((≠)(depot_path), DEPOT_PATH)
         rm(load_path, recursive=true, force=true)
-        try
-            rm(depot_path, force=true, recursive=true)
-        catch err
-            @show err
-        end
     end
 end
 

test/loading.jl

@@ -34,6 +34,7 @@ end
 @test @nested_LINE_expansion2() == ((@__LINE__() - 5, @__LINE__() - 9), @__LINE__())
 
 original_depot_path = copy(Base.DEPOT_PATH)
+include("tempdepot.jl")
 include("precompile_utils.jl")
 
 loaded_files = String[]
@@ -226,7 +227,6 @@ end
     end
 end
 
-
 ## functional testing of package identification, location & loading ##
 
 saved_load_path = copy(LOAD_PATH)
@@ -236,8 +236,9 @@ watcher_counter = Ref(0)
 push!(Base.active_project_callbacks, () -> watcher_counter[] += 1)
 push!(Base.active_project_callbacks, () -> error("broken"))
 
+const testdefaultdepot = mkdepottempdir()
 push!(empty!(LOAD_PATH), joinpath(@__DIR__, "project"))
-append!(empty!(DEPOT_PATH), [mktempdir(), joinpath(@__DIR__, "depot")])
+append!(empty!(DEPOT_PATH), [testdefaultdepot, joinpath(@__DIR__, "depot")])
 @test watcher_counter[] == 0
 @test_logs (:error, r"active project callback .* failed") Base.set_active_project(nothing)
 @test watcher_counter[] == 1
@@ -461,7 +462,7 @@ function make_env(flat, root, roots, graph, paths, dummies)
     )
 end
 
-const depots = [mktempdir() for _ = 1:3]
+const depots = [mkdepottempdir() for _ = 1:3]
 const envs = Dict{String,Any}()
 
 append!(empty!(DEPOT_PATH), depots)
@@ -755,13 +756,6 @@ end
 for env in keys(envs)
     rm(env, force=true, recursive=true)
 end
-for depot in depots
-    try
-        rm(depot, force=true, recursive=true)
-    catch err
-        @show err
-    end
-end
 
 append!(empty!(LOAD_PATH), saved_load_path)
 append!(empty!(DEPOT_PATH), saved_depot_path)
@@ -1043,9 +1037,10 @@ end
         _pkgversion == pkgversion(parent) || error("unexpected extension \$ext version: \$_pkgversion")
     end
     """
-    depot_path = mktempdir()
-    try
-        proj = joinpath(@__DIR__, "project", "Extensions", "HasDepWithExtensions.jl")
+    depot_path = mkdepottempdir()
+    proj = joinpath(@__DIR__, "project", "Extensions", "HasDepWithExtensions.jl")
+
+    begin
 
         function gen_extension_cmd(compile, distr=false)
             load_distr = distr ? "using Distributed; addprocs(1)" : ""
@@ -1155,7 +1150,7 @@ end
 
         # Extension-to-extension dependencies
 
-        mktempdir() do depot # Parallel pre-compilation
+        mkdepottempdir() do depot # Parallel pre-compilation
             code = """
             Base.disable_parallel_precompile = false
             using ExtToExtDependency
@@ -1171,7 +1166,7 @@ end
             )
             @test occursin("Hello ext-to-ext!", String(read(cmd)))
         end
-        mktempdir() do depot # Serial pre-compilation
+        mkdepottempdir() do depot # Serial pre-compilation
             code = """
             Base.disable_parallel_precompile = true
             using ExtToExtDependency
@@ -1188,7 +1183,7 @@ end
             @test occursin("Hello ext-to-ext!", String(read(cmd)))
         end
 
-        mktempdir() do depot # Parallel pre-compilation
+        mkdepottempdir() do depot # Parallel pre-compilation
             code = """
             Base.disable_parallel_precompile = false
             using CrossPackageExtToExtDependency
@@ -1204,7 +1199,7 @@ end
             )
             @test occursin("Hello x-package ext-to-ext!", String(read(cmd)))
         end
-        mktempdir() do depot # Serial pre-compilation
+        mkdepottempdir() do depot # Serial pre-compilation
             code = """
             Base.disable_parallel_precompile = true
             using CrossPackageExtToExtDependency
@@ -1224,7 +1219,7 @@ end
         # Extensions for "parent" dependencies
         # (i.e. an `ExtAB`  where A depends on / loads B, but B provides the extension)
 
-        mktempdir() do depot # Parallel pre-compilation
+        mkdepottempdir() do depot # Parallel pre-compilation
             code = """
             Base.disable_parallel_precompile = false
             using Parent
@@ -1239,7 +1234,7 @@ end
             )
             @test occursin("Hello parent!", String(read(cmd)))
         end
-        mktempdir() do depot # Serial pre-compilation
+        mkdepottempdir() do depot # Serial pre-compilation
             code = """
             Base.disable_parallel_precompile = true
             using Parent
@@ -1254,13 +1249,6 @@ end
             )
             @test occursin("Hello parent!", String(read(cmd)))
         end
-
-    finally
-        try
-            rm(depot_path, force=true, recursive=true)
-        catch err
-            @show err
-        end
     end
 end
 
@@ -1364,7 +1352,7 @@ end
 end
 
 @testset "relocatable upgrades #51989" begin
-    mktempdir() do depot
+    mkdepottempdir() do depot
         # realpath is needed because Pkg is used for one of the precompile paths below, and Pkg calls realpath on the
         # project path so the cache file slug will be different if the tempdir is given as a symlink
         # (which it often is on MacOS) which would break the test.
@@ -1446,7 +1434,7 @@ end
 end
 
 @testset "code coverage disabled during precompilation" begin
-    mktempdir() do depot
+    mkdepottempdir() do depot
         cov_test_dir = joinpath(@__DIR__, "project", "deps", "CovTest.jl")
         cov_cache_dir = joinpath(depot, "compiled", "v$(VERSION.major).$(VERSION.minor)", "CovTest")
         function rm_cov_files()
@@ -1490,7 +1478,7 @@ end
 end
 
 @testset "command-line flags" begin
-    mktempdir() do depot_path mktempdir() do dir
+    mkdepottempdir() do depot_path mktempdir() do dir
         # generate a Parent.jl and Child.jl package, with Parent depending on Child
         open(joinpath(dir, "Child.jl"), "w") do io
             println(io, """
@@ -1573,7 +1561,7 @@ end
 end
 
 @testset "including non-existent file throws proper error #52462" begin
-    mktempdir() do depot
+    mkdepottempdir() do depot
         project_path = joinpath(depot, "project")
         mkpath(project_path)
 
@@ -1715,7 +1703,7 @@ end
 end
 
 @testset "require_stdlib loading duplication" begin
-    depot_path = mktempdir()
+    depot_path = mkdepottempdir()
     oldBase64 = nothing
     try
         push!(empty!(DEPOT_PATH), depot_path)
@@ -1739,7 +1727,6 @@ end
     finally
         oldBase64 === nothing || Base.register_root_module(oldBase64)
         copy!(DEPOT_PATH, original_depot_path)
-        rm(depot_path, force=true, recursive=true)
     end
 end
 

test/precompile.jl

@@ -4,6 +4,7 @@ using Test, Distributed, Random, Logging, Libdl
 using REPL # testing the doc lookup function should be outside of the scope of this file, but is currently tested here
 
 include("precompile_utils.jl")
+include("tempdepot.jl")
 
 Foo_module = :Foo4b3a94a1a081a8cb
 foo_incl_dep = :foo4b3a94a1a081a8cb
@@ -1524,11 +1525,11 @@ end
     test_workers = addprocs(1)
     push!(test_workers, myid())
     save_cwd = pwd()
-    temp_path = mktempdir()
+    temp_path = mkdepottempdir()
     try
         cd(temp_path)
         load_path = mktempdir(temp_path)
-        load_cache_path = mktempdir(temp_path)
+        load_cache_path = mkdepottempdir(temp_path)
 
         ModuleA = :Issue19960A
         ModuleB = :Issue19960B
@@ -1576,11 +1577,6 @@ end
         end
     finally
         cd(save_cwd)
-        try
-            rm(temp_path, recursive=true)
-        catch err
-            @show err
-        end
         pop!(test_workers) # remove myid
         rmprocs(test_workers)
     end

test/precompile_utils.jl

@@ -1,28 +1,18 @@
 # This file is a part of Julia. License is MIT: https://julialang.org/license
 
+include("tempdepot.jl")
+
 function precompile_test_harness(@nospecialize(f), testset::String)
     @testset "$testset" precompile_test_harness(f, true)
 end
 function precompile_test_harness(@nospecialize(f), separate::Bool=true)
-    load_path = mktempdir()
-    load_cache_path = separate ? mktempdir() : load_path
+    load_path = mkdepottempdir()
+    load_cache_path = separate ? mkdepottempdir() : load_path
     try
         pushfirst!(LOAD_PATH, load_path)
         pushfirst!(DEPOT_PATH, load_cache_path)
         f(load_path)
     finally
-        try
-            rm(load_path, force=true, recursive=true)
-        catch err
-            @show err
-        end
-        if separate
-            try
-                rm(load_cache_path, force=true, recursive=true)
-            catch err
-                @show err
-            end
-        end
         filter!((≠)(load_path), LOAD_PATH)
         separate && filter!((≠)(load_cache_path), DEPOT_PATH)
     end