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? #52897

Closed
ericphanson opened this issue Jan 13, 2024 · 2 comments · Fixed by #52898
Closed

Add Lockable to Base? #52897

ericphanson opened this issue Jan 13, 2024 · 2 comments · Fixed by #52898

Comments

@ericphanson
Copy link
Contributor

e.g. like

struct Lockable{T, L <: Base.AbstractLock}
    value::T
    lock::L
end
Lockable(value) = Lockable(value, ReentrantLock())
Base.getindex(l::Lockable) = l.value

as in https://github.com/JuliaServices/ConcurrentUtilities.jl/blob/main/src/lockable.jl.

I think this could be ergonomic for use in Base since there's various bits of global state that each need a lock (e.g. #49778), many of which are of the form

const RESOURCE = ...
const RESOURCE_LOCK = ReentrantLock()

and one does @lock(RESOURCE_LOCK, ...access RESOURCE...). With Lockable, they could be

const RESOURCE = Lockable(...)

and used by @lock(RESOURCE, ...access RESOURCE[]...).

By grouping the resource and it's lock into a struct, it makes it slightly harder to accidentally use the resource without the lock (since one needs to index into it), and it makes it crystal clear what lock is for what resource.

@DilumAluthge
Copy link
Member

Kind of similar to #34400

I don't remember why we ended up closing that one.

@ericphanson
Copy link
Contributor Author

Ah yes, identical! 😄. I can only find one place to use this in base currently (TEMP_CLEANUP), although I suspect there should be more somewhere

vtjnash pushed a commit that referenced this issue Feb 12, 2024
I am not sure about a `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`. We didn't add them both
as usages yet, to avoid external breakage from delaying this PR.

Similarly, this is not exported yet, to avoid breakage with older
releases of ConcurrentUtilities.jl.

redo of #34400
First commit copied from
https://github.com/JuliaServices/ConcurrentUtilities.jl/blob/main/src/lockable.jl,
Closes #52897 

Co-authored-by: Jacob Quinn <quinn.jacobd@gmail.com>
Co-authored-by: Dilum Aluthge <dilum@aluthge.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants