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

Add Lockable to Base, to bundle a lock with its resource #52898

Merged
merged 20 commits into from
Feb 12, 2024

Conversation

ericphanson
Copy link
Contributor

redo of #34400
closes #52897

I think this can be useful, so putting it back up. (First commit I copied from https://github.com/JuliaServices/ConcurrentUtilities.jl/blob/main/src/lockable.jl, so I added @quinnj as a co-author, then I realized it came from #34400, so I added @DilumAluthge).

I am not sure about the lock(f, ::Lockable) method: it is nice because it unpacks the value for you, but it is weird because it unpacks the value for you. For a Lockable, f must accept one argument, whereas for a Lock, f must be 0-arg. A Lockable is not <:AbstractLock here, so maybe this is allowed, but if we deleted this lock method, we could inherit from AbstractLock and just use the generic one (requiring folks to unpack the value within the locked region themselves, as is the case for @lock). I think it is preferred these days to use @lock anyway, so having the lock(f,::Lockable) method may be of limited value anyway.

I searched Base and came up with two places that could currently use this internally, TEMP_CLEANUP and env_dict, so I've added commits to show the usage there.

base/lock.jl Outdated Show resolved Hide resolved
base/lock.jl Outdated Show resolved Hide resolved
Copy link
Member

@vtjnash vtjnash left a comment

Choose a reason for hiding this comment

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

SGTM

NEWS.md Outdated Show resolved Hide resolved
@ericphanson
Copy link
Contributor Author

ericphanson commented Jan 15, 2024

ArgTools tests are failing here, should be fixed by JuliaIO/ArgTools.jl#35. Alternatively, we could remove the TEMP_CLEANUP changes from this PR.

base/lock.jl Show resolved Hide resolved
@vtjnash
Copy link
Member

vtjnash commented Jan 30, 2024

Would probably be best not to hold up this PR for that change, but is almost done now, so can just see about rebasing onto soon: #53124

@vtjnash vtjnash added merge me PR is reviewed. Merge when all tests are passing multithreading Base.Threads and related functionality labels Jan 30, 2024
@inkydragon
Copy link
Member

inkydragon commented Feb 1, 2024

Test failed related:

Error in testset Pkg:
Error During Test at /Users/julia/.julia/scratchspaces/a66863c6-20e8-4ff4-8a62-49f30b1f605e/agent-cache/default-honeycrisp-XG3Q6T6R70.0/build/default-honeycrisp-XG3Q6T6R70-0/julialang/julia-master/julia-7f8a4a525c/share/julia/test/testdefs.jl:24
  Got exception outside of a @test
  LoadError: MethodError: no method matching empty!(::Lockable{Dict{String, Bool}, ReentrantLock})
  Closest candidates are:
    ...
  Stacktrace:
    [1] top-level scope
      @ /Users/julia/.julia/scratchspaces/a66863c6-20e8-4ff4-8a62-49f30b1f605e/agent-cache/default-honeycrisp-XG3Q6T6R70.0/build/default-honeycrisp-XG3Q6T6R70-0/julialang/julia-master/julia-7f8a4a525c/share/julia/stdlib/v1.11/Pkg/test/runtests.jl:96

https://github.com/JuliaLang/Pkg.jl/blob/f3b81f1aac77acf08f5d847ead29ad0a228dec67/test/runtests.jl#L94-L99

@IanButterworth IanButterworth removed the merge me PR is reviewed. Merge when all tests are passing label Feb 1, 2024
@ericphanson
Copy link
Contributor Author

Urg, I regret making the temp file changes here, I just wanted to show Lockable had uses within Base. At least I checked if TEMP_FILE isa Lockable in the ArgTools PR so it's not too late to revert that here.

@ericphanson
Copy link
Contributor Author

Failure seems unrelated now:

cmdlineargs                                       (7) |         failed at 2024-02-04T00:12:08.906
Test Failed at /cache/build/tester-amdci4-12/julialang/julia-master/julia-8a35ded8ae/share/julia/test/cmdlineargs.jl:1065
  Expression: success(pipeline(setenv(`$(Base.julia_cmd()) --bug-report=rr-local -e 'exit()'`, "JULIA_RR_RECORD_ARGS" => "-n --nested=ignore", "_RR_TRACE_DIR" => temp_trace_dir); ))

@vtjnash
Copy link
Member

vtjnash commented Feb 6, 2024

Seems odd it would fail repeatedly though, when that test hasn't usually been a problem

@vtjnash
Copy link
Member

vtjnash commented Feb 6, 2024

Indeed it is related. Running locally gives:

WARNING: both ConcurrentUtilities and Base export "Lockable"; uses of it in module Connections must be qualified
ERROR: LoadError: UndefVarError: `Lockable` not defined in `HTTP.Connections`
Stacktrace:
 [1] top-level scope
   @ ~/.julia/packages/HTTP/bDoga/src/Connections.jl:385
 [2] include(mod::Module, _path::String)
   @ Base ./Base.jl:556
 [3] include(x::String)
   @ HTTP ~/.julia/packages/HTTP/bDoga/src/HTTP.jl:1
 [4] top-level scope
   @ ~/.julia/packages/HTTP/bDoga/src/HTTP.jl:40
 [5] include
   @ ./Base.jl:556 [inlined]
 [6] include_package_for_output(pkg::Base.PkgId, input::String, depot_path::Vector{String}, dl_load_path::Vector{String}, load_path::Vector{String}, concrete_deps::Vector{Pair{Base.PkgId, UInt128}}, source::String)
   @ Base ./loading.jl:2427
 [7] top-level scope
   @ stdin:3
in expression starting at /home/vtjnash/.julia/packages/HTTP/bDoga/src/Connections.jl:1
in expression starting at /home/vtjnash/.julia/packages/HTTP/bDoga/src/HTTP.jl:1
in expression starting at stdin:3

@DilumAluthge
Copy link
Member

Sounds like HTTP.jl has a using ConcurrentUtilities somewhere that they need to change to using ConcurrentUtilities: Lockable.

@vtjnash
Copy link
Member

vtjnash commented Feb 6, 2024

Or ConcurrentUtilities should add a case if isdefined(Base, :Lockable); using Base: Lockable; else; include("lockable.jl"); end

vtjnash added a commit to vtjnash/ConcurrentUtilities.jl that referenced this pull request Feb 6, 2024
@vtjnash
Copy link
Member

vtjnash commented Feb 6, 2024

JuliaServices/ConcurrentUtilities.jl#31

@adienes
Copy link
Contributor

adienes commented Feb 8, 2024

would be cool to have this in 1.11 if possible

@vtjnash
Copy link
Member

vtjnash commented Feb 8, 2024

I don't have the ability to make a new JuliaServices/ConcurrentUtilities.jl#31 release, but once that is done, this should be able to merge

base/exports.jl Outdated Show resolved Hide resolved
test/misc.jl Show resolved Hide resolved
@vtjnash vtjnash added the merge me PR is reviewed. Merge when all tests are passing label Feb 8, 2024
@vtjnash
Copy link
Member

vtjnash commented Feb 8, 2024

You know what though, we don't actually have to wait for that. We will put off doing the actual export for a later day though.

quinnj pushed a commit to JuliaServices/ConcurrentUtilities.jl that referenced this pull request Feb 8, 2024
@quinnj
Copy link
Member

quinnj commented Feb 8, 2024

submitted a new ConcurrentUtilities.jl release

@vtjnash
Copy link
Member

vtjnash commented Feb 8, 2024

Thanks! We will still merge this today, and then now can merge the export in a couple days or weeks to give people time to update to the new release

@vtjnash vtjnash merged commit 57f02bf into JuliaLang:master Feb 12, 2024
4 of 7 checks passed
@inkydragon inkydragon removed the merge me PR is reviewed. Merge when all tests are passing label Feb 12, 2024
@ericphanson ericphanson deleted the eph/lockable branch February 12, 2024 10:35
@ericphanson
Copy link
Contributor Author

Thanks for getting this in @vtjnash!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
multithreading Base.Threads and related functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Lockable to Base?