From 418866f6c1d22ab6d20c2c265176b9d6ca5f1065 Mon Sep 17 00:00:00 2001 From: Tim Holy Date: Wed, 25 Sep 2024 04:08:34 -0500 Subject: [PATCH] Add locking to `revise()` Fixes #837, fixes #845 --- src/packagedef.jl | 189 +++++++++++++++++++++++++--------------------- src/recipes.jl | 92 +++++++++++----------- 2 files changed, 149 insertions(+), 132 deletions(-) diff --git a/src/packagedef.jl b/src/packagedef.jl index 00cfc79e..990e7c11 100644 --- a/src/packagedef.jl +++ b/src/packagedef.jl @@ -85,6 +85,9 @@ Global variable, a set of `Manifest.toml` files from the active projects used du """ const watched_manifests = Set{String}() +# Locks access to `revision_queue` to prevent race conditions, see issues #837 and #845 +const revise_lock = ReentrantLock() + """ Revise.revision_queue @@ -586,9 +589,11 @@ This is generally called via a [`Revise.TaskThunk`](@ref). end if id != NOPACKAGE pkgdata = pkgdatas[id] - if hasfile(pkgdata, key) # issue #228 - push!(revision_queue, (pkgdata, relpath(key, pkgdata))) - notify(revision_event) + lock(revise_lock) do + if hasfile(pkgdata, key) # issue #228 + push!(revision_queue, (pkgdata, relpath(key, pkgdata))) + notify(revision_event) + end end end end @@ -639,7 +644,11 @@ function revise_file_queued(pkgdata::PkgData, file) # Check to see if we're still watching this file stillwatching = haskey(watched_files, dirfull) - PkgId(pkgdata) != NOPACKAGE && push!(revision_queue, (pkgdata, relpath(file, pkgdata))) + if PkgId(pkgdata) != NOPACKAGE + lock(revise_lock) do + push!(revision_queue, (pkgdata, relpath(file, pkgdata))) + end + end end return end @@ -734,8 +743,10 @@ end Attempt to perform previously-failed revisions. This can be useful in cases of order-dependent errors. """ function retry() - for (k, v) in queue_errors - push!(revision_queue, k) + lock(revise_lock) do + for (k, v) in queue_errors + push!(revision_queue, k) + end end revise() end @@ -749,100 +760,102 @@ otherwise these are only logged. """ function revise(; throw=false) sleep(0.01) # in case the file system isn't quite done writing out the new files - have_queue_errors = !isempty(queue_errors) - - # Do all the deletion first. This ensures that a method that moved from one file to another - # won't get redefined first and deleted second. - revision_errors = Tuple{PkgData,String}[] - queue = sort!(collect(revision_queue); lt=pkgfileless) - finished = eltype(revision_queue)[] - mexsnews = ModuleExprsSigs[] - interrupt = false - for (pkgdata, file) in queue - try - push!(mexsnews, handle_deletions(pkgdata, file)[1]) - push!(finished, (pkgdata, file)) - catch err - throw && Base.throw(err) - interrupt |= isa(err, InterruptException) - push!(revision_errors, (pkgdata, file)) - queue_errors[(pkgdata, file)] = (err, catch_backtrace()) + lock(revise_lock) do + have_queue_errors = !isempty(queue_errors) + + # Do all the deletion first. This ensures that a method that moved from one file to another + # won't get redefined first and deleted second. + revision_errors = Tuple{PkgData,String}[] + queue = sort!(collect(revision_queue); lt=pkgfileless) + finished = eltype(revision_queue)[] + mexsnews = ModuleExprsSigs[] + interrupt = false + for (pkgdata, file) in queue + try + push!(mexsnews, handle_deletions(pkgdata, file)[1]) + push!(finished, (pkgdata, file)) + catch err + throw && Base.throw(err) + interrupt |= isa(err, InterruptException) + push!(revision_errors, (pkgdata, file)) + queue_errors[(pkgdata, file)] = (err, catch_backtrace()) + end end - end - # Do the evaluation - for ((pkgdata, file), mexsnew) in zip(finished, mexsnews) - defaultmode = PkgId(pkgdata).name == "Main" ? :evalmeth : :eval - i = fileindex(pkgdata, file) - i === nothing && continue # file was deleted by `handle_deletions` - fi = fileinfo(pkgdata, i) - modsremaining = Set(keys(mexsnew)) - changed, err = true, nothing - while changed - changed = false - for (mod, exsnew) in mexsnew - mod ∈ modsremaining || continue - try - mode = defaultmode - # Allow packages to override the supplied mode - if isdefined(mod, :__revise_mode__) - mode = getfield(mod, :__revise_mode__)::Symbol - end - mode ∈ (:sigs, :eval, :evalmeth, :evalassign) || error("unsupported mode ", mode) - exsold = get(fi.modexsigs, mod, empty_exs_sigs) - for rex in keys(exsnew) - sigs, includes = eval_rex(rex, exsold, mod; mode=mode) - if sigs !== nothing - exsnew[rex] = sigs + # Do the evaluation + for ((pkgdata, file), mexsnew) in zip(finished, mexsnews) + defaultmode = PkgId(pkgdata).name == "Main" ? :evalmeth : :eval + i = fileindex(pkgdata, file) + i === nothing && continue # file was deleted by `handle_deletions` + fi = fileinfo(pkgdata, i) + modsremaining = Set(keys(mexsnew)) + changed, err = true, nothing + while changed + changed = false + for (mod, exsnew) in mexsnew + mod ∈ modsremaining || continue + try + mode = defaultmode + # Allow packages to override the supplied mode + if isdefined(mod, :__revise_mode__) + mode = getfield(mod, :__revise_mode__)::Symbol end - if includes !== nothing - maybe_add_includes_to_pkgdata!(pkgdata, file, includes; eval_now=true) + mode ∈ (:sigs, :eval, :evalmeth, :evalassign) || error("unsupported mode ", mode) + exsold = get(fi.modexsigs, mod, empty_exs_sigs) + for rex in keys(exsnew) + sigs, includes = eval_rex(rex, exsold, mod; mode=mode) + if sigs !== nothing + exsnew[rex] = sigs + end + if includes !== nothing + maybe_add_includes_to_pkgdata!(pkgdata, file, includes; eval_now=true) + end end + delete!(modsremaining, mod) + changed = true + catch _err + err = _err end - delete!(modsremaining, mod) - changed = true - catch _err - err = _err end end + if isempty(modsremaining) + pkgdata.fileinfos[i] = FileInfo(mexsnew, fi) + delete!(queue_errors, (pkgdata, file)) + else + throw && Base.throw(err) + interrupt |= isa(err, InterruptException) + push!(revision_errors, (pkgdata, file)) + queue_errors[(pkgdata, file)] = (err, catch_backtrace()) + end end - if isempty(modsremaining) - pkgdata.fileinfos[i] = FileInfo(mexsnew, fi) - delete!(queue_errors, (pkgdata, file)) + if interrupt + for pkgfile in finished + haskey(queue_errors, pkgfile) || delete!(revision_queue, pkgfile) + end else - throw && Base.throw(err) - interrupt |= isa(err, InterruptException) - push!(revision_errors, (pkgdata, file)) - queue_errors[(pkgdata, file)] = (err, catch_backtrace()) + empty!(revision_queue) end - end - if interrupt - for pkgfile in finished - haskey(queue_errors, pkgfile) || delete!(revision_queue, pkgfile) - end - else - empty!(revision_queue) - end - errors(revision_errors) - if !isempty(queue_errors) - if !have_queue_errors # only print on the first time errors occur - io = IOBuffer() - println(io, "\n") # better here than in the triple-quoted literal, see https://github.com/JuliaLang/julia/issues/34105 - for (pkgdata, file) in keys(queue_errors) - println(io, " ", joinpath(basedir(pkgdata), file)) + errors(revision_errors) + if !isempty(queue_errors) + if !have_queue_errors # only print on the first time errors occur + io = IOBuffer() + println(io, "\n") # better here than in the triple-quoted literal, see https://github.com/JuliaLang/julia/issues/34105 + for (pkgdata, file) in keys(queue_errors) + println(io, " ", joinpath(basedir(pkgdata), file)) + end + str = String(take!(io)) + @warn """The running code does not match the saved version for the following files:$str + If the error was due to evaluation order, it can sometimes be resolved by calling `Revise.retry()`. + Use Revise.errors() to report errors again. Only the first error in each file is shown. + Your prompt color may be yellow until the errors are resolved.""" + maybe_set_prompt_color(:warn) end - str = String(take!(io)) - @warn """The running code does not match the saved version for the following files:$str - If the error was due to evaluation order, it can sometimes be resolved by calling `Revise.retry()`. - Use Revise.errors() to report errors again. Only the first error in each file is shown. - Your prompt color may be yellow until the errors are resolved.""" - maybe_set_prompt_color(:warn) + else + maybe_set_prompt_color(:ok) end - else - maybe_set_prompt_color(:ok) - end - tracking_Main_includes[] && queue_includes(Main) + tracking_Main_includes[] && queue_includes(Main) - process_user_callbacks!(throw=throw) + process_user_callbacks!(throw=throw) + end nothing end diff --git a/src/recipes.jl b/src/recipes.jl index 41bbab3b..39596a84 100644 --- a/src/recipes.jl +++ b/src/recipes.jl @@ -61,22 +61,24 @@ function _track(id, modname; modified_files=revision_queue) if pkgdata === nothing pkgdata = PkgData(id, srcdir) end - for (submod, filename) in Iterators.drop(Base._included_files, 1) # stepping through sysimg.jl rebuilds Base, omit it - ffilename = fixpath(filename) - inpath(ffilename, dirs) || continue - keypath = ffilename[1:last(findfirst(dirs[end], ffilename))] - rpath = relpath(ffilename, keypath) - fullpath = joinpath(basedir(pkgdata), rpath) - if fullpath != filename - cache_file_key[fullpath] = filename - src_file_key[filename] = fullpath - end - push!(pkgdata, rpath=>FileInfo(submod, basesrccache)) - if mtime(ffilename) > mtcache - with_logger(_debug_logger) do - @debug "Recipe for Base/StdLib" _group="Watching" filename=filename mtime=mtime(filename) mtimeref=mtcache + lock(revise_lock) do + for (submod, filename) in Iterators.drop(Base._included_files, 1) # stepping through sysimg.jl rebuilds Base, omit it + ffilename = fixpath(filename) + inpath(ffilename, dirs) || continue + keypath = ffilename[1:last(findfirst(dirs[end], ffilename))] + rpath = relpath(ffilename, keypath) + fullpath = joinpath(basedir(pkgdata), rpath) + if fullpath != filename + cache_file_key[fullpath] = filename + src_file_key[filename] = fullpath + end + push!(pkgdata, rpath=>FileInfo(submod, basesrccache)) + if mtime(ffilename) > mtcache + with_logger(_debug_logger) do + @debug "Recipe for Base/StdLib" _group="Watching" filename=filename mtime=mtime(filename) mtimeref=mtcache + end + push!(modified_files, (pkgdata, rpath)) end - push!(modified_files, (pkgdata, rpath)) end end # Add files to CodeTracking pkgfiles @@ -135,39 +137,41 @@ function track_subdir_from_git!(pkgdata::PkgData, subdir::AbstractString; commit tree = git_tree(repo, commit) files = Iterators.filter(file->startswith(file, prefix) && endswith(file, ".jl"), keys(tree)) ccall((:giterr_clear, :libgit2), Cvoid, ()) # necessary to avoid errors like "the global/xdg file 'attributes' doesn't exist: No such file or directory" - for file in files - fullpath = joinpath(repo_path, file) - rpath = relpath(fullpath, pkgdata) # this might undo the above, except for Core.Compiler - local src - try - src = git_source(file, tree) - catch err - if err isa KeyError - @warn "skipping $file, not found in repo" - continue + lock(revise_lock) do + for file in files + fullpath = joinpath(repo_path, file) + rpath = relpath(fullpath, pkgdata) # this might undo the above, except for Core.Compiler + local src + try + src = git_source(file, tree) + catch err + if err isa KeyError + @warn "skipping $file, not found in repo" + continue + end + rethrow(err) end - rethrow(err) - end - fmod = get(juliaf2m, fullpath, Core.Compiler) # Core.Compiler is not cached - if fmod === Core.Compiler - endswith(fullpath, "compiler.jl") && continue # defines the module, skip - @static if isdefined(Core.Compiler, :EscapeAnalysis) - # after https://github.com/JuliaLang/julia/pull/43800 - if contains(fullpath, "compiler/ssair/EscapeAnalysis") - fmod = Core.Compiler.EscapeAnalysis + fmod = get(juliaf2m, fullpath, Core.Compiler) # Core.Compiler is not cached + if fmod === Core.Compiler + endswith(fullpath, "compiler.jl") && continue # defines the module, skip + @static if isdefined(Core.Compiler, :EscapeAnalysis) + # after https://github.com/JuliaLang/julia/pull/43800 + if contains(fullpath, "compiler/ssair/EscapeAnalysis") + fmod = Core.Compiler.EscapeAnalysis + end end end + if src != read(fullpath, String) + push!(modified_files, (pkgdata, rpath)) + end + fi = FileInfo(fmod) + if parse_source!(fi.modexsigs, src, file, fmod) === nothing + @warn "failed to parse Git source text for $file" + else + instantiate_sigs!(fi.modexsigs) + end + push!(pkgdata, rpath=>fi) end - if src != read(fullpath, String) - push!(modified_files, (pkgdata, rpath)) - end - fi = FileInfo(fmod) - if parse_source!(fi.modexsigs, src, file, fmod) === nothing - @warn "failed to parse Git source text for $file" - else - instantiate_sigs!(fi.modexsigs) - end - push!(pkgdata, rpath=>fi) end if !isempty(pkgdata.fileinfos) id = PkgId(pkgdata)