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.

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

Fix manifest_usage.toml and Registry process concurrency via pid locking #2793

Merged
merged 8 commits into from
Mar 10, 2022

Conversation

IanButterworth
Copy link
Member

@IanButterworth IanButterworth commented Oct 24, 2021

Implements pidfile locking for manifest_usage.toml and Registry add/update operations

Fixes #2875
Fixes #2346
Fixes #684
Fixes #2746
Fixes #1664

Includes the process concurrency test from #2732 which would cause Pkg master to error in two ways

  • the env usage file mv breaking
  • the registry update mv breaking

cc. @vtjnash

@IanButterworth
Copy link
Member Author

IanButterworth commented Oct 24, 2021

I think this is success, but one issue remains that seems to be a Windows bug that's intermittent and unrelated, but the new tests seem to make it more likely to happen..

  Activating new project at `C:\Users\runneradmin\AppData\Local\Temp\jl_JdOnsj`
┌ Info: waiting for lock on pidfile
└   path = "D:\\a\\Pkg.jl\\Pkg.jl\\test\\tmp\\registries\\.pidlock"
ERROR: IOError: unlink("tmp\\registries\\General.tar.gz"): resource busy or locked (EBUSY)
Stacktrace:
  [1] uv_error
    @ .\libuv.jl:97 [inlined]
  [2] unlink(p::String)
    @ Base.Filesystem .\file.jl:970
  [3] rm(path::String; force::Bool, recursive::Bool)
    @ Base.Filesystem .\file.jl:281
  [4] checkfor_mv_cp_cptree(src::String, dst::String, txt::String; force::Bool)
    @ Base.Filesystem .\file.jl:328
  [5] #mv#15
    @ .\file.jl:423 [inlined]
  [6] (::Pkg.Registry.var"#37#40"{Base.DevNull, Vector{Pkg.Registry.RegistrySpec}, String})()
    @ Pkg.Registry D:\a\Pkg.jl\Pkg.jl\src\Registry\Registry.jl:184
  [7] mkpidlock(f::Pkg.Registry.var"#37#40"{Base.DevNull, Vector{Pkg.Registry.RegistrySpec}, String}, at::String, pid::Int32; kwopts::Base.Pairs{Symbol, Union{}, Tuple{}, NamedTuple{(), Tuple{}}})
    @ Pkg.Pidfile D:\a\Pkg.jl\Pkg.jl\src\pidfile.jl:64
  [8] mkpidlock
    @ D:\a\Pkg.jl\Pkg.jl\src\pidfile.jl:62 [inlined]
  [9] #mkpidlock#4
    @ D:\a\Pkg.jl\Pkg.jl\src\pidfile.jl:59 [inlined]
 [10] mkpidlock
    @ D:\a\Pkg.jl\Pkg.jl\src\pidfile.jl:59 [inlined]
 [11] download_registries(io::Base.DevNull, regs::Vector{Pkg.Registry.RegistrySpec}, depot::String)
    @ Pkg.Registry D:\a\Pkg.jl\Pkg.jl\src\Registry\Registry.jl:165

which looks like JuliaLang/julia#29658 and JuliaLang/julia#39457

@vtjnash
Copy link
Member

vtjnash commented Oct 25, 2021

You also might wish to enable stale_age for automatic lock breaking

@IanButterworth
Copy link
Member Author

Ok! I allowed 30 seconds for writing the env usage file, given it might in a bad case take ~1 second.

For registry download/update I allowed 1 hr, given its download dependent. I'm less certain of that being the correct setting because exorbitantly slow registry updates could happen on a slow connection.
It might be painful if a user loses track of a process that's stuck doing a slow registry update and they keep hitting the pid lock message elsewhere..

@vtjnash
Copy link
Member

vtjnash commented Oct 26, 2021

That is good. It will also check the pid validity and wait 25x longer if it still seems to exist

@IanButterworth
Copy link
Member Author

Oh I'll walk them back a bit then given the 25x.

Perhaps now would be a good time to move Pidfile.jl to the JuliaLang org, if you wouldn't mind doing that?

@StefanKarpinski
Copy link
Member

Can we rename the module to PidFiles to conform with how we name packages these days?

@IanButterworth IanButterworth added this to the 1.8 milestone Jan 2, 2022
@IanButterworth IanButterworth changed the title Try fixing process concurrency with Pidfile Fix process concurrency via internally vendored Pidfile Feb 12, 2022
@IanButterworth
Copy link
Member Author

The MacOS failure is #2596

Copy link
Member

@fredrikekre fredrikekre left a comment

Choose a reason for hiding this comment

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

Great! Could Pkg also use a lockfile for precompilation?

@DilumAluthge
Copy link
Member

DilumAluthge commented Feb 12, 2022

ERROR: LoadError: ArgumentError: Package Pkg does not have FileWatching in its dependencies:

@IanButterworth Could you update the manifest file at ext/Historical.../Manifest.toml? That will fix the "Check Historical..." CI failure.

@DilumAluthge
Copy link
Member

@IanButterworth Would this be a good opportunity to upgrade the ext/Historical.../Manifest.toml file to the new manifest format? 😉

@DilumAluthge
Copy link
Member

party_parrot.jpg

@IanButterworth
Copy link
Member Author

Could Pkg also use a lockfile for precompilation?

Could be, but there's a currently a bug with ctrl-c-ing pkg precompilation that causes julia to fatally error, that I've not been able to track down. and if we pidlocked the precompile process, I believe that would leave the lock file around until it was deemed stale, which might be hard to find the right balance for.

Perhaps that could be a followup PR, ideally after that bug has been squashed?

@IanButterworth IanButterworth changed the title Fix process concurrency via internally vendored Pidfile Fix process concurrency via FileWatching.mkpidfile Feb 28, 2022
@IanButterworth
Copy link
Member Author

IanButterworth commented Feb 28, 2022

Needs to wait for JuliaLang/julia#44367 to land on the nightlies cc. @vtjnash

@IanButterworth IanButterworth changed the title Fix process concurrency via FileWatching.mkpidfile Fix process concurrency via FileWatching.mkpidlock Feb 28, 2022
@IanButterworth IanButterworth modified the milestones: 1.8, 1.9 Mar 9, 2022
src/Registry/Registry.jl Outdated Show resolved Hide resolved
src/Registry/Registry.jl Outdated Show resolved Hide resolved
src/Registry/Registry.jl Outdated Show resolved Hide resolved
src/Registry/Registry.jl Outdated Show resolved Hide resolved
src/Registry/Registry.jl Outdated Show resolved Hide resolved
src/Registry/Registry.jl Outdated Show resolved Hide resolved
@IanButterworth IanButterworth changed the title Fix process concurrency via FileWatching.mkpidlock Fix manifest_usage.toml and Registry process concurrency via pid locking Mar 10, 2022
@mbauman
Copy link
Member

mbauman commented Jul 1, 2022

Could this be backported to 1.8?

@KristofferC
Copy link
Member

I don't think we have PID locking available in Base on 1.8.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment