From 9d7d27bc24169021d1d4494cffa1d82245b29559 Mon Sep 17 00:00:00 2001 From: Jameson Nash Date: Sun, 23 Jun 2024 04:18:56 +0000 Subject: [PATCH 1/2] fix concurrent module loading return value Previously this might return `nothing` which would confuse the caller of `start_loading` which expects that to mean the Module didn't load. It is not entirely clear if this code ever worked, even single-threaded. Fix #54813 --- base/loading.jl | 33 ++++++++++++++++++--------------- test/threads_exec.jl | 8 +++++--- 2 files changed, 23 insertions(+), 18 deletions(-) diff --git a/base/loading.jl b/base/loading.jl index 1c4eb2235b31d..c5d710f4868e2 100644 --- a/base/loading.jl +++ b/base/loading.jl @@ -1996,8 +1996,12 @@ debug_loading_deadlocks::Bool = true # Enable a slightly more expensive, but mor function start_loading(modkey::PkgId) # handle recursive calls to require assert_havelock(require_lock) - loading = get(package_locks, modkey, nothing) - if loading !== nothing + while true + loading = get(package_locks, modkey, nothing) + if loading === nothing + package_locks[modkey] = current_task() => Threads.Condition(require_lock) + return nothing + end # load already in progress for this module on the task task, cond = loading deps = String[modkey.name] @@ -2036,10 +2040,9 @@ function start_loading(modkey::PkgId) end throw(ConcurrencyViolationError(msg)) end - return wait(cond) + loading = wait(cond) + loading isa Module && return loading end - package_locks[modkey] = current_task() => Threads.Condition(require_lock) - return end function end_loading(modkey::PkgId, @nospecialize loaded) @@ -2418,9 +2421,9 @@ function _require(pkg::PkgId, env=nothing) # attempt to load the module file via the precompile cache locations if JLOptions().use_compiled_modules != 0 @label load_from_cache - m = _require_search_from_serialized(pkg, path, UInt128(0), true; reasons) - if m isa Module - return m + loaded = _require_search_from_serialized(pkg, path, UInt128(0), true; reasons) + if loaded isa Module + return loaded end end @@ -2456,14 +2459,14 @@ function _require(pkg::PkgId, env=nothing) @goto load_from_cache end # spawn off a new incremental pre-compile task for recursive `require` calls - cachefile_or_module = maybe_cachefile_lock(pkg, path) do + loaded = maybe_cachefile_lock(pkg, path) do # double-check now that we have lock m = _require_search_from_serialized(pkg, path, UInt128(0), true) m isa Module && return m - compilecache(pkg, path; reasons) + return compilecache(pkg, path; reasons) end - cachefile_or_module isa Module && return cachefile_or_module::Module - cachefile = cachefile_or_module + loaded isa Module && return loaded + cachefile = loaded if isnothing(cachefile) # maybe_cachefile_lock returns nothing if it had to wait for another process @goto load_from_cache # the new cachefile will have the newest mtime so will come first in the search elseif isa(cachefile, Exception) @@ -2476,11 +2479,11 @@ function _require(pkg::PkgId, env=nothing) # fall-through to loading the file locally if not incremental else cachefile, ocachefile = cachefile::Tuple{String, Union{Nothing, String}} - m = _tryrequire_from_serialized(pkg, cachefile, ocachefile) - if !isa(m, Module) + loaded = _tryrequire_from_serialized(pkg, cachefile, ocachefile) + if !isa(loaded, Module) @warn "The call to compilecache failed to create a usable precompiled cache file for $(repr("text/plain", pkg))" exception=m else - return m + return loaded end end if JLOptions().incremental != 0 diff --git a/test/threads_exec.jl b/test/threads_exec.jl index 7f4775cae0055..595f8991d58d7 100644 --- a/test/threads_exec.jl +++ b/test/threads_exec.jl @@ -1122,23 +1122,25 @@ end # issue #41546, thread-safe package loading @testset "package loading" begin - ch = Channel{Bool}(threadpoolsize()) + ntasks = max(threadpoolsize(), 4) + ch = Channel{Bool}(ntasks) barrier = Base.Event() old_act_proj = Base.ACTIVE_PROJECT[] try pushfirst!(LOAD_PATH, "@") Base.ACTIVE_PROJECT[] = joinpath(@__DIR__, "TestPkg") @sync begin - for _ in 1:threadpoolsize() + for _ in 1:ntasks Threads.@spawn begin put!(ch, true) wait(barrier) @eval using TestPkg end end - for _ in 1:threadpoolsize() + for _ in 1:ntasks take!(ch) end + close(ch) notify(barrier) end @test Base.root_module(@__MODULE__, :TestPkg) isa Module From e78a342ff6fb505bfc4e5d1fdcec6e61e40d4296 Mon Sep 17 00:00:00 2001 From: Jameson Nash Date: Mon, 24 Jun 2024 20:00:39 +0000 Subject: [PATCH 2/2] fixup! fix concurrent module loading return value --- base/loading.jl | 17 ++++++++--------- test/loading.jl | 17 +++++++++++------ 2 files changed, 19 insertions(+), 15 deletions(-) diff --git a/base/loading.jl b/base/loading.jl index c5d710f4868e2..4d6c190f00d22 100644 --- a/base/loading.jl +++ b/base/loading.jl @@ -2460,28 +2460,27 @@ function _require(pkg::PkgId, env=nothing) end # spawn off a new incremental pre-compile task for recursive `require` calls loaded = maybe_cachefile_lock(pkg, path) do - # double-check now that we have lock + # double-check the search now that we have lock m = _require_search_from_serialized(pkg, path, UInt128(0), true) m isa Module && return m return compilecache(pkg, path; reasons) end loaded isa Module && return loaded - cachefile = loaded - if isnothing(cachefile) # maybe_cachefile_lock returns nothing if it had to wait for another process + if isnothing(loaded) # maybe_cachefile_lock returns nothing if it had to wait for another process @goto load_from_cache # the new cachefile will have the newest mtime so will come first in the search - elseif isa(cachefile, Exception) - if precompilableerror(cachefile) + elseif isa(loaded, Exception) + if precompilableerror(loaded) verbosity = isinteractive() ? CoreLogging.Info : CoreLogging.Debug - @logmsg verbosity "Skipping precompilation due to precompilable error. Importing $(repr("text/plain", pkg))." exception=m + @logmsg verbosity "Skipping precompilation due to precompilable error. Importing $(repr("text/plain", pkg))." exception=loaded else - @warn "The call to compilecache failed to create a usable precompiled cache file for $(repr("text/plain", pkg))" exception=m + @warn "The call to compilecache failed to create a usable precompiled cache file for $(repr("text/plain", pkg))" exception=loaded end # fall-through to loading the file locally if not incremental else - cachefile, ocachefile = cachefile::Tuple{String, Union{Nothing, String}} + cachefile, ocachefile = loaded::Tuple{String, Union{Nothing, String}} loaded = _tryrequire_from_serialized(pkg, cachefile, ocachefile) if !isa(loaded, Module) - @warn "The call to compilecache failed to create a usable precompiled cache file for $(repr("text/plain", pkg))" exception=m + @warn "The call to compilecache failed to create a usable precompiled cache file for $(repr("text/plain", pkg))" exception=loaded else return loaded end diff --git a/test/loading.jl b/test/loading.jl index 72589d4210adc..41b5cc9596f7d 100644 --- a/test/loading.jl +++ b/test/loading.jl @@ -1210,6 +1210,11 @@ end empty!(Base.DEPOT_PATH) append!(Base.DEPOT_PATH, original_depot_path) +module loaded_pkgid1 end +module loaded_pkgid2 end +module loaded_pkgid3 end +module loaded_pkgid4 end + @testset "loading deadlock detector" begin pkid1 = Base.PkgId("pkgid1") pkid2 = Base.PkgId("pkgid2") @@ -1221,16 +1226,16 @@ append!(Base.DEPOT_PATH, original_depot_path) t1 = @async begin @test nothing === @lock Base.require_lock Base.start_loading(pkid2) # @async module pkgid2; using pkgid1; end notify(e) - @test "loaded_pkgid1" == @lock Base.require_lock Base.start_loading(pkid1) - @lock Base.require_lock Base.end_loading(pkid2, "loaded_pkgid2") + @test loaded_pkgid1 == @lock Base.require_lock Base.start_loading(pkid1) + @lock Base.require_lock Base.end_loading(pkid2, loaded_pkgid2) end wait(e) reset(e) t2 = @async begin @test nothing === @lock Base.require_lock Base.start_loading(pkid3) # @async module pkgid3; using pkgid2; end notify(e) - @test "loaded_pkgid2" == @lock Base.require_lock Base.start_loading(pkid2) - @lock Base.require_lock Base.end_loading(pkid3, "loaded_pkgid3") + @test loaded_pkgid2 == @lock Base.require_lock Base.start_loading(pkid2) + @lock Base.require_lock Base.end_loading(pkid3, loaded_pkgid3) end wait(e) reset(e) @@ -1238,8 +1243,8 @@ append!(Base.DEPOT_PATH, original_depot_path) @lock Base.require_lock Base.start_loading(pkid3)).value # try using pkgid3 @test_throws(ConcurrencyViolationError("deadlock detected in loading pkgid4 -> pkgid4 && pkgid1"), @lock Base.require_lock Base.start_loading(pkid4)).value # try using pkgid4 - @lock Base.require_lock Base.end_loading(pkid1, "loaded_pkgid1") # end - @lock Base.require_lock Base.end_loading(pkid4, "loaded_pkgid4") # end + @lock Base.require_lock Base.end_loading(pkid1, loaded_pkgid1) # end + @lock Base.require_lock Base.end_loading(pkid4, loaded_pkgid4) # end wait(t2) wait(t1) end