Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[REPL] fix lock ordering mistake in load_pkg #56215

Merged
merged 1 commit into from
Oct 17, 2024
Merged

[REPL] fix lock ordering mistake in load_pkg #56215

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions base/loading.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2093,6 +2093,7 @@ debug_loading_deadlocks::Bool = true # Enable a slightly more expensive, but mor
function start_loading(modkey::PkgId, build_id::UInt128, stalecheck::Bool)
# handle recursive and concurrent calls to require
assert_havelock(require_lock)
require_lock.reentrancy_cnt == 1 || throw(ConcurrencyViolationError("recursive call to start_loading"))
while true
loaded = stalecheck ? maybe_root_module(modkey) : nothing
loaded isa Module && return loaded
Expand Down
7 changes: 3 additions & 4 deletions stdlib/REPL/src/Pkg_beforeload.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,17 +1,16 @@
## Pkg stuff needed before Pkg has loaded

const Pkg_pkgid = Base.PkgId(Base.UUID("44cfe95a-1eb2-52ea-b672-e2afdf69b78f"), "Pkg")
const Pkg_REPLExt_pkgid = Base.PkgId(Base.UUID("ceef7b17-42e7-5b1c-81d4-4cc4a2494ccf"), "REPLExt")

function load_pkg()
REPLExt = Base.require_stdlib(Pkg_pkgid, "REPLExt")
@lock Base.require_lock begin
REPLExt = Base.require_stdlib(Pkg_pkgid, "REPLExt")
Comment on lines +6 to -8
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

require_stdlib locks require_lock for the entire function, so why does the lock need to be released between these operations?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It releases the lock many times internally

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah so the additional lock layer prevents those from releasing fully

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. We could do an unlockall/relockall pair like wait does, but it is sort of always a concurrency bug if your application actually relies on that.

# require_stdlib does not guarantee that the `__init__` of the package is done when loading is done async
# but we need to wait for the repl mode to be set up
lock = get(Base.package_locks, Pkg_REPLExt_pkgid.uuid, nothing)
lock = get(Base.package_locks, Base.PkgId(REPLExt), nothing)
lock !== nothing && wait(lock[2])
return REPLExt
end
return REPLExt
end

## Below here copied/tweaked from Pkg Types.jl so that the dummy Pkg prompt
Expand Down