Skip to content

Commit

Permalink
Extensions: make loading of extensions independent of what packages a…
Browse files Browse the repository at this point in the history
…re in the sysimage (#52841)

When triggers of extension are in the sysimage it is easy to end up with
cycles in package loading. Say we have a package A with exts BExt and
CExt and say that B and C is in the sysimage.

- Upon loading A, we will immidiately start to precompile BExt (because
the trigger B is "loaded" by virtue of being in the sysimage).
- BExt will load A which will cause CExt to start precompiling (again
because C is in the sysimage).
- CExt will load A which will now cause BExt to start loading and we get
a cycle.

This is fixed in this PR by instead of looking at what modules are
loaded, we look at what modules are actually `require`d and only use
that to drive the loading of extensions.

Fixes #52132.
  • Loading branch information
KristofferC authored Jan 29, 2024
1 parent 9669eec commit 08d229f
Show file tree
Hide file tree
Showing 6 changed files with 40 additions and 3 deletions.
1 change: 1 addition & 0 deletions base/Base.jl
Original file line number Diff line number Diff line change
Expand Up @@ -627,6 +627,7 @@ function __init__()
init_load_path()
init_active_project()
append!(empty!(_sysimage_modules), keys(loaded_modules))
empty!(explicit_loaded_modules)
if haskey(ENV, "JULIA_MAX_NUM_PRECOMPILE_FILES")
MAX_NUM_PRECOMPILE_FILES[] = parse(Int, ENV["JULIA_MAX_NUM_PRECOMPILE_FILES"])
end
Expand Down
10 changes: 9 additions & 1 deletion base/loading.jl
Original file line number Diff line number Diff line change
Expand Up @@ -1354,7 +1354,7 @@ function _insert_extension_triggers(parent::PkgId, extensions::Dict{String, Any}
# TODO: Better error message if this lookup fails?
uuid_trigger = UUID(weakdeps[trigger]::String)
trigger_id = PkgId(uuid_trigger, trigger)
if !haskey(Base.loaded_modules, trigger_id) || haskey(package_locks, trigger_id)
if !haskey(explicit_loaded_modules, trigger_id) || haskey(package_locks, trigger_id)
trigger1 = get!(Vector{ExtensionId}, EXT_DORMITORY, trigger_id)
push!(trigger1, gid)
else
Expand Down Expand Up @@ -1986,6 +1986,11 @@ function __require_prelocked(uuidkey::PkgId, env=nothing)
# After successfully loading, notify downstream consumers
run_package_callbacks(uuidkey)
else
m = get(loaded_modules, uuidkey, nothing)
if m !== nothing
explicit_loaded_modules[uuidkey] = m
run_package_callbacks(uuidkey)
end
newm = root_module(uuidkey)
end
return newm
Expand All @@ -2000,6 +2005,8 @@ PkgOrigin() = PkgOrigin(nothing, nothing, nothing)
const pkgorigins = Dict{PkgId,PkgOrigin}()

const loaded_modules = Dict{PkgId,Module}()
# Emptied on Julia start
const explicit_loaded_modules = Dict{PkgId,Module}()
const loaded_modules_order = Vector{Module}()
const module_keys = IdDict{Module,PkgId}() # the reverse

Expand All @@ -2023,6 +2030,7 @@ root_module_key(m::Module) = @lock require_lock module_keys[m]
end
push!(loaded_modules_order, m)
loaded_modules[key] = m
explicit_loaded_modules[key] = m
module_keys[m] = key
end
nothing
Expand Down
18 changes: 18 additions & 0 deletions test/loading.jl
Original file line number Diff line number Diff line change
Expand Up @@ -1113,6 +1113,24 @@ end
cmd_proj_ext = addenv(cmd_proj_ext, "JULIA_LOAD_PATH" => join([joinpath(proj, "HasExtensions.jl"), joinpath(proj, "EnvWithDeps")], sep))
run(cmd_proj_ext)
end

# Sysimage extensions
# The test below requires that LinearAlgebra is in the sysimage and that it has not been loaded yet.
# if it gets moved out, this test will need to be updated.
# We run this test in a new process so we are not vulnerable to a previous test having loaded LinearAlgebra
sysimg_ext_test_code = """
uuid_key = Base.PkgId(Base.UUID("37e2e46d-f89d-539d-b4ee-838fcccc9c8e"), "LinearAlgebra")
Base.in_sysimage(uuid_key) || error("LinearAlgebra not in sysimage")
haskey(Base.explicit_loaded_modules, uuid_key) && error("LinearAlgebra already loaded")
using HasExtensions
Base.get_extension(HasExtensions, :LinearAlgebraExt) === nothing || error("unexpectedly got an extension")
using LinearAlgebra
haskey(Base.explicit_loaded_modules, uuid_key) || error("LinearAlgebra not loaded")
Base.get_extension(HasExtensions, :LinearAlgebraExt) isa Module || error("expected extension to load")
"""
cmd = `$(Base.julia_cmd()) --startup-file=no -e $sysimg_ext_test_code`
cmd = addenv(cmd, "JULIA_LOAD_PATH" => join([proj, "@stdlib"], sep))
run(cmd)
finally
try
rm(depot_path, force=true, recursive=true)
Expand Down
9 changes: 7 additions & 2 deletions test/project/Extensions/HasDepWithExtensions.jl/Manifest.toml
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
# This file is machine-generated - editing it directly is not advised

julia_version = "1.10.0-DEV"
julia_version = "1.10.0"
manifest_format = "2.0"
project_hash = "d523b3401f72a1ed34b7b43749fd2655c6b78542"

Expand All @@ -19,11 +19,16 @@ version = "0.1.0"
path = "../HasExtensions.jl"
uuid = "4d3288b3-3afc-4bb6-85f3-489fffe514c8"
version = "0.1.0"
weakdeps = ["ExtDep", "ExtDep2"]

[deps.HasExtensions.extensions]
Extension = "ExtDep"
ExtensionFolder = ["ExtDep", "ExtDep2"]
LinearAlgebraExt = "LinearAlgebra"

[deps.HasExtensions.weakdeps]
ExtDep = "fa069be4-f60b-4d4c-8b95-f8008775090c"
ExtDep2 = "55982ee5-2ad5-4c40-8cfe-5e9e1b01500d"
LinearAlgebra = "37e2e46d-f89d-539d-b4ee-838fcccc9c8e"

[[deps.SomePackage]]
path = "../SomePackage"
Expand Down
2 changes: 2 additions & 0 deletions test/project/Extensions/HasExtensions.jl/Project.toml
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,9 @@ version = "0.1.0"
[weakdeps]
ExtDep = "fa069be4-f60b-4d4c-8b95-f8008775090c"
ExtDep2 = "55982ee5-2ad5-4c40-8cfe-5e9e1b01500d"
LinearAlgebra = "37e2e46d-f89d-539d-b4ee-838fcccc9c8e"

[extensions]
Extension = "ExtDep"
ExtensionFolder = ["ExtDep", "ExtDep2"]
LinearAlgebraExt = "LinearAlgebra"
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
module LinearAlgebraExt

end

0 comments on commit 08d229f

Please sign in to comment.