From 62cb02b0e4127d7a5dfc3b658eff7a38704e85dd Mon Sep 17 00:00:00 2001
From: Jameson Nash <vtjnash@gmail.com>
Date: Sun, 23 Jun 2024 04:18:56 +0000
Subject: [PATCH] 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 f10ed3907d17c..6b8afaaba2707 100644
--- a/base/loading.jl
+++ b/base/loading.jl
@@ -1995,8 +1995,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]
@@ -2035,10 +2039,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)
@@ -2417,9 +2420,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
 
@@ -2455,14 +2458,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)
@@ -2475,11 +2478,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