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

[Serialization] support AbstractLock and GenericCondition #43325

Merged
merged 1 commit into from
Dec 17, 2021

Conversation

vtjnash
Copy link
Member

@vtjnash vtjnash commented Dec 3, 2021

Locks should be unlocked when serialized, and GenericCondition should
drop their waiters.

Otherwise, we may try to serialize a running Task (the user should
normally be holding the lock around the data they are intending to
serialize), which will fail and error.

Discovered in trying to make #42339 work

Locks should be unlocked when serialized, and GenericCondition should
drop their waiters.

Otherwise, we may try to serialize a running Task (the user should
normally be holding the lock around the data they are intending to
serialize), which will fail and error.
@vtjnash vtjnash requested a review from tkf December 3, 2021 19:28
Comment on lines +1531 to +1532
function deserialize(s::AbstractSerializer, ::Type{T}) where T<:Base.AbstractLock
lock = T()
Copy link
Member

@tkf tkf Dec 3, 2021

Choose a reason for hiding this comment

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

This assumes that all fields of an AbstractLock are lock states, right? I think it's conceivable to have locks with parameters (e.g., back-off delays), and these values have to be serialized. Maybe we can serialize only const fields? It's somewhat too magical but kinda cool too.

In a long run, it'd be nice to have a Serialization API to declare the part of the value that shouldn't be serialized. I needed a rather ugly hack to avoid serialization of process-local scratch space:
https://github.com/JuliaFolds/FLoops.jl/blob/255382d84c187ec10521c0166412e5dfb213471b/src/scratchspace.jl#L45-L48 I don't think we need to solve it here but AbstractLock sounds like a good case study.

Copy link
Member Author

Choose a reason for hiding this comment

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

Talked to Jeff, and we discussed how this is better than the current situation, and if someone needs something fancier, they can still implement that.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I agree this PR is an improvement overall.

@vtjnash vtjnash added the merge me PR is reviewed. Merge when all tests are passing label Dec 17, 2021
@krynju
Copy link
Contributor

krynju commented Dec 17, 2021

Should we revert this Future copy on serialization once this is merged?
This was added to avoid serializing a potentially locked lock

fc = Future((f.where, f.whence, f.id, v_cache)) # copy to be used for serialization (contains a reset lock)

@tkf tkf merged commit eea8b14 into master Dec 17, 2021
@tkf tkf deleted the jn/serialize-locks branch December 17, 2021 23:50
@tkf tkf added multithreading Base.Threads and related functionality parallelism Parallel or distributed computation and removed merge me PR is reviewed. Merge when all tests are passing labels Dec 17, 2021
@tkf
Copy link
Member

tkf commented Dec 17, 2021

@krynju Maybe open a PR or an issue and ping @vtjnash there so that we don't forget about this?

LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Feb 22, 2022
…43325)

Locks should be unlocked when serialized, and GenericCondition should
drop their waiters.

Otherwise, we may try to serialize a running Task (the user should
normally be holding the lock around the data they are intending to
serialize), which will fail and error.
LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Mar 8, 2022
…43325)

Locks should be unlocked when serialized, and GenericCondition should
drop their waiters.

Otherwise, we may try to serialize a running Task (the user should
normally be holding the lock around the data they are intending to
serialize), which will fail and error.
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 parallelism Parallel or distributed computation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants