Revert "fix #41546, make using thread-safe (#41602)"

This reverts commit e3255ef.
JuliaLang · Oct 29, 2021 · 901a3a5 · 901a3a5
1 parent 21a2784
commit 901a3a5
Show file tree

Hide file tree

Showing 4 changed files with 27 additions and 84 deletions.
diff --git a/base/loading.jl b/base/loading.jl
@@ -1,7 +1,6 @@
 # This file is a part of Julia. License is MIT: https://julialang.org/license
 
 # Base.require is the implementation for the `import` statement
-const require_lock = ReentrantLock()
 
 # Cross-platform case-sensitive path canonicalization
 
@@ -130,7 +129,6 @@ end
 const ns_dummy_uuid = UUID("fe0723d6-3a44-4c41-8065-ee0f42c8ceab")
 
 function dummy_uuid(project_file::String)
-    @lock require_lock begin
     cache = LOADING_CACHE[]
     if cache !== nothing
         uuid = get(cache.dummy_uuid, project_file, nothing)
@@ -146,7 +144,6 @@ function dummy_uuid(project_file::String)
         cache.dummy_uuid[project_file] = uuid
     end
     return uuid
-    end
 end
 
 ## package path slugs: turning UUID + SHA1 into a pair of 4-byte "slugs" ##
@@ -239,7 +236,8 @@ struct TOMLCache
 end
 const TOML_CACHE = TOMLCache(TOML.Parser(), Dict{String, Dict{String, Any}}())
 
-parsed_toml(project_file::AbstractString) = parsed_toml(project_file, TOML_CACHE, require_lock)
+const TOML_LOCK = ReentrantLock()
+parsed_toml(project_file::AbstractString) = parsed_toml(project_file, TOML_CACHE, TOML_LOCK)
 function parsed_toml(project_file::AbstractString, toml_cache::TOMLCache, toml_lock::ReentrantLock)
     lock(toml_lock) do
         cache = LOADING_CACHE[]
@@ -339,15 +337,13 @@ Use [`dirname`](@ref) to get the directory part and [`basename`](@ref)
 to get the file name part of the path.
 """
 function pathof(m::Module)
-    @lock require_lock begin
-    pkgid = get(module_keys, m, nothing)
+    pkgid = get(Base.module_keys, m, nothing)
     pkgid === nothing && return nothing
-    origin = get(pkgorigins, pkgid, nothing)
+    origin = get(Base.pkgorigins, pkgid, nothing)
     origin === nothing && return nothing
     path = origin.path
     path === nothing && return nothing
     return fixup_stdlib_path(path)
-    end
 end
 
 """
@@ -370,7 +366,7 @@ julia> pkgdir(Foo, "src", "file.jl")
     The optional argument `paths` requires at least Julia 1.7.
 """
 function pkgdir(m::Module, paths::String...)
-    rootmodule = moduleroot(m)
+    rootmodule = Base.moduleroot(m)
     path = pathof(rootmodule)
     path === nothing && return nothing
     return joinpath(dirname(dirname(path)), paths...)
@@ -387,7 +383,6 @@ const preferences_names = ("JuliaLocalPreferences.toml", "LocalPreferences.toml"
 #  - `true`: `env` is an implicit environment
 #  - `path`: the path of an explicit project file
 function env_project_file(env::String)::Union{Bool,String}
-    @lock require_lock begin
     cache = LOADING_CACHE[]
     if cache !== nothing
         project_file = get(cache.env_project_file, env, nothing)
@@ -411,7 +406,6 @@ function env_project_file(env::String)::Union{Bool,String}
         cache.env_project_file[env] = project_file
     end
     return project_file
-    end
 end
 
 function project_deps_get(env::String, name::String)::Union{Nothing,PkgId}
@@ -479,7 +473,6 @@ end
 
 # find project file's corresponding manifest file
 function project_file_manifest_path(project_file::String)::Union{Nothing,String}
-    @lock require_lock begin
     cache = LOADING_CACHE[]
     if cache !== nothing
         manifest_path = get(cache.project_file_manifest_path, project_file, missing)
@@ -508,7 +501,6 @@ function project_file_manifest_path(project_file::String)::Union{Nothing,String}
         cache.project_file_manifest_path[project_file] = manifest_path
     end
     return manifest_path
-    end
 end
 
 # given a directory (implicit env from LOAD_PATH) and a name,
@@ -696,7 +688,7 @@ function implicit_manifest_deps_get(dir::String, where::PkgId, name::String)::Un
     @assert where.uuid !== nothing
     project_file = entry_point_and_project_file(dir, where.name)[2]
     project_file === nothing && return nothing # a project file is mandatory for a package with a uuid
-    proj = project_file_name_uuid(project_file, where.name)
+    proj = project_file_name_uuid(project_file, where.name, )
     proj == where || return nothing # verify that this is the correct project file
     # this is the correct project, so stop searching here
     pkg_uuid = explicit_project_deps_get(project_file, name)
@@ -761,26 +753,19 @@ function _include_from_serialized(path::String, depmods::Vector{Any})
     if isa(sv, Exception)
         return sv
     end
-    sv = sv::SimpleVector
-    restored = sv[1]::Vector{Any}
-    for M in restored
-        M = M::Module
-        if isdefined(M, Base.Docs.META)
-            push!(Base.Docs.modules, M)
-        end
-        if parentmodule(M) === M
-            register_root_module(M)
-        end
-    end
-    inits = sv[2]::Vector{Any}
-    if !isempty(inits)
-        unlock(require_lock) # temporarily _unlock_ during these callbacks
-        try
-            ccall(:jl_init_restored_modules, Cvoid, (Any,), inits)
-        finally
-            lock(require_lock)
+    restored = sv[1]
+    if !isa(restored, Exception)
+        for M in restored::Vector{Any}
+            M = M::Module
+            if isdefined(M, Base.Docs.META)
+                push!(Base.Docs.modules, M)
+            end
+            if parentmodule(M) === M
+                register_root_module(M)
+            end
         end
     end
+    isassigned(sv, 2) && ccall(:jl_init_restored_modules, Cvoid, (Any,), sv[2])
     return restored
 end
 
@@ -877,7 +862,7 @@ function _require_search_from_serialized(pkg::PkgId, sourcepath::String)
 end
 
 # to synchronize multiple tasks trying to import/using something
-const package_locks = Dict{PkgId,Threads.Condition}()
+const package_locks = Dict{PkgId,Condition}()
 
 # to notify downstream consumers that a module was successfully loaded
 # Callbacks take the form (mod::Base.PkgId) -> nothing.
@@ -900,9 +885,7 @@ function _include_dependency(mod::Module, _path::AbstractString)
         path = normpath(joinpath(dirname(prev), _path))
     end
     if _track_dependencies[]
-        @lock require_lock begin
         push!(_require_dependencies, (mod, path, mtime(path)))
-        end
     end
     return path, prev
 end
@@ -974,7 +957,6 @@ For more details regarding code loading, see the manual sections on [modules](@r
 [parallel computing](@ref code-availability).
 """
 function require(into::Module, mod::Symbol)
-    @lock require_lock begin
     LOADING_CACHE[] = LoadingCache()
     try
         uuidkey = identify_package(into, String(mod))
@@ -1016,7 +998,6 @@ function require(into::Module, mod::Symbol)
     finally
         LOADING_CACHE[] = nothing
     end
-    end
 end
 
 mutable struct PkgOrigin
@@ -1028,7 +1009,6 @@ PkgOrigin() = PkgOrigin(nothing, nothing)
 const pkgorigins = Dict{PkgId,PkgOrigin}()
 
 function require(uuidkey::PkgId)
-    @lock require_lock begin
     if !root_module_exists(uuidkey)
         cachefile = _require(uuidkey)
         if cachefile !== nothing
@@ -1040,19 +1020,15 @@ function require(uuidkey::PkgId)
         end
     end
     return root_module(uuidkey)
-    end
 end
 
 const loaded_modules = Dict{PkgId,Module}()
 const module_keys = IdDict{Module,PkgId}() # the reverse
 
-is_root_module(m::Module) = @lock require_lock haskey(module_keys, m)
-root_module_key(m::Module) = @lock require_lock module_keys[m]
+is_root_module(m::Module) = haskey(module_keys, m)
+root_module_key(m::Module) = module_keys[m]
 
 function register_root_module(m::Module)
-    # n.b. This is called from C after creating a new module in `Base.__toplevel__`,
-    # instead of adding them to the binding table there.
-    @lock require_lock begin
     key = PkgId(m, String(nameof(m)))
     if haskey(loaded_modules, key)
         oldm = loaded_modules[key]
@@ -1062,7 +1038,6 @@ function register_root_module(m::Module)
     end
     loaded_modules[key] = m
     module_keys[m] = key
-    end
     nothing
 end
 
@@ -1078,13 +1053,12 @@ using Base
 end
 
 # get a top-level Module from the given key
-root_module(key::PkgId) = @lock require_lock loaded_modules[key]
+root_module(key::PkgId) = loaded_modules[key]
 root_module(where::Module, name::Symbol) =
     root_module(identify_package(where, String(name)))
-maybe_root_module(key::PkgId) = @lock require_lock get(loaded_modules, key, nothing)
 
-root_module_exists(key::PkgId) = @lock require_lock haskey(loaded_modules, key)
-loaded_modules_array() = @lock require_lock collect(values(loaded_modules))
+root_module_exists(key::PkgId) = haskey(loaded_modules, key)
+loaded_modules_array() = collect(values(loaded_modules))
 
 function unreference_module(key::PkgId)
     if haskey(loaded_modules, key)
@@ -1103,7 +1077,7 @@ function _require(pkg::PkgId)
         wait(loading)
         return
     end
-    package_locks[pkg] = Threads.Condition(require_lock)
+    package_locks[pkg] = Condition()
 
     last = toplevel_load[]
     try
@@ -1171,12 +1145,10 @@ function _require(pkg::PkgId)
         if uuid !== old_uuid
             ccall(:jl_set_module_uuid, Cvoid, (Any, NTuple{2, UInt64}), __toplevel__, uuid)
         end
-        unlock(require_lock)
         try
             include(__toplevel__, path)
             return
         finally
-            lock(require_lock)
             if uuid !== old_uuid
                 ccall(:jl_set_module_uuid, Cvoid, (Any, NTuple{2, UInt64}), __toplevel__, old_uuid)
             end

diff --git a/base/toml_parser.jl b/base/toml_parser.jl
@@ -104,7 +104,7 @@ function Parser(str::String; filepath=nothing)
             IdSet{TOMLDict}(),    # defined_tables
             root,
             filepath,
-            isdefined(Base, :maybe_root_module) ? Base.maybe_root_module(DATES_PKGID) : nothing,
+            isdefined(Base, :loaded_modules) ? get(Base.loaded_modules, DATES_PKGID, nothing) : nothing,
         )
     startup(l)
     return l

diff --git a/src/dump.c b/src/dump.c
@@ -2203,6 +2203,8 @@ static jl_array_t *jl_finalize_deserializer(jl_serializer_state *s, arraylist_t
 
 JL_DLLEXPORT void jl_init_restored_modules(jl_array_t *init_order)
 {
+    if (!init_order)
+        return;
     int i, l = jl_array_len(init_order);
     for (i = 0; i < l; i++) {
         jl_value_t *mod = jl_array_ptr_ref(init_order, i);
@@ -2655,9 +2657,6 @@ static jl_value_t *_jl_restore_incremental(ios_t *f, jl_array_t *mod_array)
     jl_recache_other(); // make all of the other objects identities correct (needs to be after insert methods)
     htable_free(&uniquing_table);
     jl_array_t *init_order = jl_finalize_deserializer(&s, tracee_list); // done with f and s (needs to be after recache)
-    if (init_order == NULL)
-        init_order = (jl_array_t*)jl_an_empty_vec_any;
-    assert(jl_isa((jl_value_t*)init_order, jl_array_any_type));
 
     JL_GC_PUSH4(&init_order, &restored, &external_backedges, &external_edges);
     jl_gc_enable(en); // subtyping can allocate a lot, not valid before recache-other

diff --git a/test/threads_exec.jl b/test/threads_exec.jl
@@ -912,31 +912,3 @@ end
         @test reproducible_rand(r, 10) == val
     end
 end
-
-# issue #41546, thread-safe package loading
-@testset "package loading" begin
-    ch = Channel{Bool}(nthreads())
-    barrier = Base.Event()
-    old_act_proj = Base.ACTIVE_PROJECT[]
-    try
-        pushfirst!(LOAD_PATH, "@")
-        Base.ACTIVE_PROJECT[] = joinpath(@__DIR__, "TestPkg")
-        @sync begin
-            for _ in 1:nthreads()
-                Threads.@spawn begin
-                    put!(ch, true)
-                    wait(barrier)
-                    @eval using TestPkg
-                end
-            end
-            for _ in 1:nthreads()
-                take!(ch)
-            end
-            notify(barrier)
-        end
-        @test Base.root_module(@__MODULE__, :TestPkg) isa Module
-    finally
-        Base.ACTIVE_PROJECT[] = old_act_proj
-        popfirst!(LOAD_PATH)
-    end
-end