Skip to content

Commit

Permalink
ensure extension triggers are only run by the package that satified them
Browse files Browse the repository at this point in the history 
Oscar had noticed a problem where loading an extension might trigger
loading another package, which might re-trigger attempting to load the
same extension. And then that causes a deadlock from waiting for the
extension to finish loading (it appears to be a recursive import
triggered from multiple places). Instead alter the representation, to be
similar to a semaphore, so that it will be loaded only exactly by the
final package that satisfied all dependencies for it.

This approach could still encounter an issue if the user imports a
package (C) which it does not explicitly list as a dependency for
extension. But it is unclear to me that we actually want to solve that,
since it weakens and delays the premise that Bext is available shortly
after A and B are both loaded.

\# module C; using A, B; end;; module A; end;; module B; end;; module Bext; using C; end
\# using C, Bext / A / B
starts C -> requires A, B to load
loads A -> defines Bext (extends B, but tries to also require C)
loads B -> loads Bext (which waits for C -> deadlock!)
finish C -> now safe to load Bext

While using this order would have been fine.
\# using A, B, Bext / C
loads A -> defines Bext (extends B, but tries to also require C)
loads B -> starts Bext
loads C
finish Bext
  • Loading branch information
@vtjnash
vtjnash committed Feb 3, 2023
1 parent bebff47 commit d7b2062
Showing 1 changed file with 85 additions and 49 deletions.
134 changes: 85 additions & 49 deletions base/loading.jl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1076,9 +1076,9 @@ function register_restored_modules(sv::SimpleVector, pkg::PkgId, path::String)
end

function run_package_callbacks(modkey::PkgId)
run_extension_callbacks(modkey)
assert_havelock(require_lock)
unlock(require_lock)
run_extension_callbacks()
try
for callback in package_callbacks
invokelatest(callback, modkey)
Expand All @@ -1099,14 +1099,13 @@ end
##############

mutable struct ExtensionId
const id::PkgId # Could be symbol?
const parentid::PkgId
const triggers::Vector{PkgId} # What packages have to be loaded for the extension to get loaded
triggered::Bool
succeeded::Bool
const id::PkgId
const parentid::PkgId # just need the name, for printing
ntriggers::Int # how many more packages must be defined until this is loaded
end

const EXT_DORMITORY = ExtensionId[]
const EXT_DORMITORY = Dict{PkgId,Vector{ExtensionId}}()
const EXT_DORMITORY_FAILED = ExtensionId[]

function insert_extension_triggers(pkg::PkgId)
pkg.uuid === nothing && return
Expand Down Expand Up @@ -1159,59 +1158,84 @@ end
function _insert_extension_triggers(parent::PkgId, extensions::Dict{String, <:Any}, weakdeps::Dict{String, <:Any})
for (ext::String, triggers::Union{String, Vector{String}}) in extensions
triggers isa String && (triggers = [triggers])
triggers_id = PkgId[]
id = PkgId(uuid5(parent.uuid, ext), ext)
gid = ExtensionId(id, parent, 1 + length(triggers))
trigger1 = get!(Vector{ExtensionId}, EXT_DORMITORY, parent)
push!(trigger1, gid)
for trigger in triggers
# TODO: Better error message if this lookup fails?
uuid_trigger = UUID(weakdeps[trigger]::String)
push!(triggers_id, PkgId(uuid_trigger, trigger))
trigger_id = PkgId(uuid_trigger, trigger)
if !haskey(Base.loaded_modules, trigger_id) || haskey(package_locks, trigger_id)
trigger1 = get!(Vector{ExtensionId}, EXT_DORMITORY, trigger_id)
push!(trigger1, gid)
else
gid.ntriggers -= 1
end
end
gid = ExtensionId(id, parent, triggers_id, false, false)
push!(EXT_DORMITORY, gid)
end
end

function run_extension_callbacks(; force::Bool=false)
try
# TODO, if `EXT_DORMITORY` becomes very long, do something smarter
for extid in EXT_DORMITORY
extid.succeeded && continue
!force && extid.triggered && continue
if all(x -> haskey(Base.loaded_modules, x) && !haskey(package_locks, x), extid.triggers)
ext_not_allowed_load = nothing
extid.triggered = true
# It is possible that some of the triggers were loaded in an environment
# below the one of the parent. This will cause a load failure when the
# pkg ext tries to load the triggers. Therefore, check this first
# before loading the pkg ext.
for trigger in extid.triggers
pkgenv = Base.identify_package_env(extid.id, trigger.name)
if pkgenv === nothing
ext_not_allowed_load = trigger
break
else
pkg, env = pkgenv
path = Base.locate_package(pkg, env)
if path === nothing
ext_not_allowed_load = trigger
break
end
end
end
if ext_not_allowed_load !== nothing
@debug "Extension $(extid.id.name) of $(extid.parentid.name) not loaded due to \
$(ext_not_allowed_load.name) loaded in environment lower in load path"
else
require(extid.id)
@debug "Extension $(extid.id.name) of $(extid.parentid.name) loaded"
end
extid.succeeded = true
end
end
function run_extension_callbacks(extid::ExtensionId)
assert_havelock(require_lock)
succeeded = try
_require_prelocked(extid.id)
@debug "Extension $(extid.id.name) of $(extid.parentid.name) loaded"
true
catch
# Try to continue loading if loading an extension errors
errs = current_exceptions()
@error "Error during loading of extension" exception=errs
false
end
return succeeded
end

function run_extension_callbacks(pkgid::PkgId)
assert_havelock(require_lock)
# take ownership of extids that depend on this pkgid
extids = pop!(EXT_DORMITORY, pkgid, nothing)
extids === nothing && return
for extid in extids
if extid.ntriggers > 0
# It is possible that pkgid was loaded in an environment
# below the one of the parent. This will cause a load failure when the
# pkg ext tries to load the triggers. Therefore, check this first
# before loading the pkg ext.
pkgenv = Base.identify_package_env(extid.id, pkgid.name)
ext_not_allowed_load = false
if pkgenv === nothing
ext_not_allowed_load = true
else
pkg, env = pkgenv
path = Base.locate_package(pkg, env)
if path === nothing
ext_not_allowed_load = true
end
end
if ext_not_allowed_load
@debug "Extension $(extid.id.name) of $(extid.parentid.name) will not be loaded \
since $(pkgid.name) loaded in environment lower in load path"
# indicate extid is expected to fail
extid.ntriggers *= -1
else
# indicate pkgid is loaded
extid.ntriggers -= 1
end
end
if extid.ntriggers < 0
# indicate pkgid is loaded
extid.ntriggers += 1
succeeded = false
else
succeeded = true
end
if extid.ntriggers == 0
# actually load extid, now that all dependencies are met,
# and record the result
succeeded = succeeded && run_extension_callbacks(extid)
succeeded || push!(EXT_DORMITORY_FAILED, extid)
end
end
nothing
end
Expand All @@ -1224,7 +1248,19 @@ This is used in cases where the automatic loading of an extension failed
due to some problem with the extension. Instead of restarting the Julia session,
the extension can be fixed, and this function run.
"""
retry_load_extensions() = run_extension_callbacks(; force=true)
function retry_load_extensions()
@lock require_lock begin
# this copy is desired since run_extension_callbacks will release this lock
# so this can still mutate the list to drop successful ones
failed = copy(EXT_DORMITORY_FAILED)
empty!(EXT_DORMITORY_FAILED)
filter!(failed) do extid
return !run_extension_callbacks(extid)
end
prepend!(EXT_DORMITORY_FAILED, failed)
end
nothing
end

"""
get_extension(parent::Module, extension::Symbol)
Expand Down

0 comments on commit d7b2062

Please sign in to comment.