Skip to content

Commit 0ef2bb6

Browse files
authored
fix concurrent module loading return value (#54898)
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
1 parent aa07585 commit 0ef2bb6

File tree

3 files changed

+41
-32
lines changed

3 files changed

+41
-32
lines changed

base/loading.jl

+25-23
Original file line numberDiff line numberDiff line change
@@ -1996,8 +1996,12 @@ debug_loading_deadlocks::Bool = true # Enable a slightly more expensive, but mor
19961996
function start_loading(modkey::PkgId)
19971997
# handle recursive calls to require
19981998
assert_havelock(require_lock)
1999-
loading = get(package_locks, modkey, nothing)
2000-
if loading !== nothing
1999+
while true
2000+
loading = get(package_locks, modkey, nothing)
2001+
if loading === nothing
2002+
package_locks[modkey] = current_task() => Threads.Condition(require_lock)
2003+
return nothing
2004+
end
20012005
# load already in progress for this module on the task
20022006
task, cond = loading
20032007
deps = String[modkey.name]
@@ -2036,10 +2040,9 @@ function start_loading(modkey::PkgId)
20362040
end
20372041
throw(ConcurrencyViolationError(msg))
20382042
end
2039-
return wait(cond)
2043+
loading = wait(cond)
2044+
loading isa Module && return loading
20402045
end
2041-
package_locks[modkey] = current_task() => Threads.Condition(require_lock)
2042-
return
20432046
end
20442047

20452048
function end_loading(modkey::PkgId, @nospecialize loaded)
@@ -2419,9 +2422,9 @@ function _require(pkg::PkgId, env=nothing)
24192422
# attempt to load the module file via the precompile cache locations
24202423
if JLOptions().use_compiled_modules != 0
24212424
@label load_from_cache
2422-
m = _require_search_from_serialized(pkg, path, UInt128(0), true; reasons)
2423-
if m isa Module
2424-
return m
2425+
loaded = _require_search_from_serialized(pkg, path, UInt128(0), true; reasons)
2426+
if loaded isa Module
2427+
return loaded
24252428
end
24262429
end
24272430

@@ -2457,31 +2460,30 @@ function _require(pkg::PkgId, env=nothing)
24572460
@goto load_from_cache
24582461
end
24592462
# spawn off a new incremental pre-compile task for recursive `require` calls
2460-
cachefile_or_module = maybe_cachefile_lock(pkg, path) do
2461-
# double-check now that we have lock
2463+
loaded = maybe_cachefile_lock(pkg, path) do
2464+
# double-check the search now that we have lock
24622465
m = _require_search_from_serialized(pkg, path, UInt128(0), true)
24632466
m isa Module && return m
2464-
compilecache(pkg, path; reasons)
2467+
return compilecache(pkg, path; reasons)
24652468
end
2466-
cachefile_or_module isa Module && return cachefile_or_module::Module
2467-
cachefile = cachefile_or_module
2468-
if isnothing(cachefile) # maybe_cachefile_lock returns nothing if it had to wait for another process
2469+
loaded isa Module && return loaded
2470+
if isnothing(loaded) # maybe_cachefile_lock returns nothing if it had to wait for another process
24692471
@goto load_from_cache # the new cachefile will have the newest mtime so will come first in the search
2470-
elseif isa(cachefile, Exception)
2471-
if precompilableerror(cachefile)
2472+
elseif isa(loaded, Exception)
2473+
if precompilableerror(loaded)
24722474
verbosity = isinteractive() ? CoreLogging.Info : CoreLogging.Debug
2473-
@logmsg verbosity "Skipping precompilation due to precompilable error. Importing $(repr("text/plain", pkg))." exception=m
2475+
@logmsg verbosity "Skipping precompilation due to precompilable error. Importing $(repr("text/plain", pkg))." exception=loaded
24742476
else
2475-
@warn "The call to compilecache failed to create a usable precompiled cache file for $(repr("text/plain", pkg))" exception=m
2477+
@warn "The call to compilecache failed to create a usable precompiled cache file for $(repr("text/plain", pkg))" exception=loaded
24762478
end
24772479
# fall-through to loading the file locally if not incremental
24782480
else
2479-
cachefile, ocachefile = cachefile::Tuple{String, Union{Nothing, String}}
2480-
m = _tryrequire_from_serialized(pkg, cachefile, ocachefile)
2481-
if !isa(m, Module)
2482-
@warn "The call to compilecache failed to create a usable precompiled cache file for $(repr("text/plain", pkg))" exception=m
2481+
cachefile, ocachefile = loaded::Tuple{String, Union{Nothing, String}}
2482+
loaded = _tryrequire_from_serialized(pkg, cachefile, ocachefile)
2483+
if !isa(loaded, Module)
2484+
@warn "The call to compilecache failed to create a usable precompiled cache file for $(repr("text/plain", pkg))" exception=loaded
24832485
else
2484-
return m
2486+
return loaded
24852487
end
24862488
end
24872489
if JLOptions().incremental != 0

test/loading.jl

+11-6
Original file line numberDiff line numberDiff line change
@@ -1210,6 +1210,11 @@ end
12101210
empty!(Base.DEPOT_PATH)
12111211
append!(Base.DEPOT_PATH, original_depot_path)
12121212

1213+
module loaded_pkgid1 end
1214+
module loaded_pkgid2 end
1215+
module loaded_pkgid3 end
1216+
module loaded_pkgid4 end
1217+
12131218
@testset "loading deadlock detector" begin
12141219
pkid1 = Base.PkgId("pkgid1")
12151220
pkid2 = Base.PkgId("pkgid2")
@@ -1221,25 +1226,25 @@ append!(Base.DEPOT_PATH, original_depot_path)
12211226
t1 = @async begin
12221227
@test nothing === @lock Base.require_lock Base.start_loading(pkid2) # @async module pkgid2; using pkgid1; end
12231228
notify(e)
1224-
@test "loaded_pkgid1" == @lock Base.require_lock Base.start_loading(pkid1)
1225-
@lock Base.require_lock Base.end_loading(pkid2, "loaded_pkgid2")
1229+
@test loaded_pkgid1 == @lock Base.require_lock Base.start_loading(pkid1)
1230+
@lock Base.require_lock Base.end_loading(pkid2, loaded_pkgid2)
12261231
end
12271232
wait(e)
12281233
reset(e)
12291234
t2 = @async begin
12301235
@test nothing === @lock Base.require_lock Base.start_loading(pkid3) # @async module pkgid3; using pkgid2; end
12311236
notify(e)
1232-
@test "loaded_pkgid2" == @lock Base.require_lock Base.start_loading(pkid2)
1233-
@lock Base.require_lock Base.end_loading(pkid3, "loaded_pkgid3")
1237+
@test loaded_pkgid2 == @lock Base.require_lock Base.start_loading(pkid2)
1238+
@lock Base.require_lock Base.end_loading(pkid3, loaded_pkgid3)
12341239
end
12351240
wait(e)
12361241
reset(e)
12371242
@test_throws(ConcurrencyViolationError("deadlock detected in loading pkgid3 -> pkgid2 -> pkgid1 -> pkgid3 && pkgid4"),
12381243
@lock Base.require_lock Base.start_loading(pkid3)).value # try using pkgid3
12391244
@test_throws(ConcurrencyViolationError("deadlock detected in loading pkgid4 -> pkgid4 && pkgid1"),
12401245
@lock Base.require_lock Base.start_loading(pkid4)).value # try using pkgid4
1241-
@lock Base.require_lock Base.end_loading(pkid1, "loaded_pkgid1") # end
1242-
@lock Base.require_lock Base.end_loading(pkid4, "loaded_pkgid4") # end
1246+
@lock Base.require_lock Base.end_loading(pkid1, loaded_pkgid1) # end
1247+
@lock Base.require_lock Base.end_loading(pkid4, loaded_pkgid4) # end
12431248
wait(t2)
12441249
wait(t1)
12451250
end

test/threads_exec.jl

+5-3
Original file line numberDiff line numberDiff line change
@@ -1122,23 +1122,25 @@ end
11221122

11231123
# issue #41546, thread-safe package loading
11241124
@testset "package loading" begin
1125-
ch = Channel{Bool}(threadpoolsize())
1125+
ntasks = max(threadpoolsize(), 4)
1126+
ch = Channel{Bool}(ntasks)
11261127
barrier = Base.Event()
11271128
old_act_proj = Base.ACTIVE_PROJECT[]
11281129
try
11291130
pushfirst!(LOAD_PATH, "@")
11301131
Base.ACTIVE_PROJECT[] = joinpath(@__DIR__, "TestPkg")
11311132
@sync begin
1132-
for _ in 1:threadpoolsize()
1133+
for _ in 1:ntasks
11331134
Threads.@spawn begin
11341135
put!(ch, true)
11351136
wait(barrier)
11361137
@eval using TestPkg
11371138
end
11381139
end
1139-
for _ in 1:threadpoolsize()
1140+
for _ in 1:ntasks
11401141
take!(ch)
11411142
end
1143+
close(ch)
11421144
notify(barrier)
11431145
end
11441146
@test Base.root_module(@__MODULE__, :TestPkg) isa Module

0 commit comments

Comments
 (0)