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

RFC: Add Lockable{T, L<:AbstractLock} struct #34400

Closed
wants to merge 3 commits into from
Closed

RFC: Add Lockable{T, L<:AbstractLock} struct #34400

wants to merge 3 commits into from

Conversation

DilumAluthge
Copy link
Member

@DilumAluthge DilumAluthge commented Jan 16, 2020

I find myself using this pattern a lot, so I'm curious if it would be worth adding to Base.

The basic idea is that there is some object that should only ever be accessed when its corresponding lock is held, so it makes sense to package the object and its lock together into a single object.

Here is the code that this PR adds to base/lock.jl:

struct Lockable{T, L<:AbstractLock}
    value::T
    lock::L
end

Lockable(value) = Lockable(value, ReentrantLock())

function lock(f, l::Lockable)
    lock(l.lock) do
        f(l.value)
    end
end

Example usage:

julia> function foo(x::Lockable)
           lock(x) do value
               push!(value, value[end-1] + value[end])
           end
       end
foo (generic function with 1 method)

julia> bar = Lockable([1, 1], ReentrantLock())

julia> foo(bar)
3-element Array{Int64,1}:
 1
 1
 2

julia> foo(bar)
4-element Array{Int64,1}:
 1
 1
 2
 3

julia> foo(bar)
5-element Array{Int64,1}:
 1
 1
 2
 3
 5

@DilumAluthge DilumAluthge changed the title Add Locked{L <: AbstractLock, T} struct RFC: Add Locked{L <: AbstractLock, T} struct Jan 16, 2020
@ararslan ararslan added needs compat annotation Add !!! compat "Julia x.y" to the docstring needs docs Documentation for this change is required needs news A NEWS entry is required for this change needs tests Unit tests are required for this change labels Jan 16, 2020
@TsurHerman
Copy link

Maybe change the name to Lockable?

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

Maybe change the name to Lockable?

Done!

@DilumAluthge
Copy link
Member Author

I have added docstrings, compat annotations, tests, and a NEWS.md entry. I've also applied all of the suggestions from code review.

@DilumAluthge DilumAluthge changed the title RFC: Add Locked{L <: AbstractLock, T} struct RFC: Add Lockable{T, L<:AbstractLock} struct Feb 5, 2020
@StefanKarpinski StefanKarpinski added the triage This should be discussed on a triage call label Feb 14, 2020
@DilumAluthge
Copy link
Member Author

Bump.

What did triage think of this PR?

base/lock.jl Show resolved Hide resolved
base/lock.jl Outdated Show resolved Hide resolved
@tkf tkf removed needs compat annotation Add !!! compat "Julia x.y" to the docstring needs docs Documentation for this change is required needs news A NEWS entry is required for this change needs tests Unit tests are required for this change labels May 13, 2020
@DilumAluthge
Copy link
Member Author

The more I think about this, the more I think this doesn't need to be in Base.

I think it is a useful design pattern, but it is easy to put this in a package instead of Base or a stdlib.

@DilumAluthge DilumAluthge deleted the dpa/locked-struct branch September 1, 2020 03:52
@StefanKarpinski StefanKarpinski removed the triage This should be discussed on a triage call label Oct 1, 2020
@quinnj
Copy link
Member

quinnj commented Oct 22, 2020

I've come across the need several times lately for something like this; does it live in a package somewhere?

@DilumAluthge
Copy link
Member Author

Not that I'm aware of. Want to take the code and submit it somewhere?

If there is some kind of "threading utilities" package, it could go there.

@quinnj
Copy link
Member

quinnj commented Oct 22, 2020

I mean, for my use case, I realized I only needed:

struct Lockable{T}
    x::T
    lock::ReentrantLock
end

Lockable(x::T) where {T} = Lockable{T}(x, ReentrantLock())

Base.lock(x::Lockable) = lock(x.lock)
Base.unlock(x::Lockable) = unlock(x.lock)

So I just put it in my package directly. But yeah, sounds like a good fit for a thread utils package.

@quinnj
Copy link
Member

quinnj commented Oct 22, 2020

Well, I was planning on registering this new package soon anyway, so now it includes Lockable.

@quinnj
Copy link
Member

quinnj commented Oct 22, 2020

https://github.com/JuliaServices/WorkerUtilities.jl

ericphanson added a commit to ericphanson/julia that referenced this pull request Jan 13, 2024
Co-authored-by: Dilum Aluthge <dilum@aluthge.com>
vtjnash pushed a commit to ericphanson/julia that referenced this pull request Jan 30, 2024
Co-authored-by: Dilum Aluthge <dilum@aluthge.com>
vtjnash pushed a commit that referenced this pull request 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 this pull request may close these issues.

6 participants