Skip to content

Conversation

@TommyXR
Copy link
Contributor

@TommyXR TommyXR commented Jan 19, 2021

lock(f, lck) is defined for locks, but not for Channels. I think it's simply a mistake, thus I added lock(f, ::Channel) for API uniformity.

@DilumAluthge DilumAluthge requested a review from aviatesk January 19, 2021 01:57
@fredrikekre fredrikekre removed their request for review January 19, 2021 05:42
@TommyXR
Copy link
Contributor Author

TommyXR commented Jan 19, 2021

Some people on the slack have argued that we should instead remove the ::ReentrantLock type restriction in the other file.

I don't mind changing the commit to reflect this, but I would like to know what people think is the better option.

@DilumAluthge
Copy link
Member

I don't feel strongly either way.

@JeffBezanson
Copy link
Member

JeffBezanson commented Jan 19, 2021

Duplicating the code is not good. There are two other methods that use this approach:

lock(f, c::GenericCondition) = lock(f, c.lock)

That's better since there's no duplication, but in fact we could just remove the ::AbstractLock restriction and remove the existing extra methods.

More weirdness:

  • lock(::WeakKeyDict) is not defined but should be (also unlock)
  • unlock(f, c::GenericCondition) = unlock(f, c.lock) is defined, but there is no similar unlock method for it to call (?)

@TommyXR
Copy link
Contributor Author

TommyXR commented Jan 20, 2021

Duplicating the code is not good. There are two other methods that use this approach:

lock(f, c::GenericCondition) = lock(f, c.lock)

That's better since there's no duplication, but in fact we could just remove the ::AbstractLock restriction and remove the existing extra methods.

More weirdness:

* `lock(::WeakKeyDict)` is not defined but should be (also `unlock`)

* `unlock(f, c::GenericCondition) = unlock(f, c.lock)` is defined, but there is no similar unlock method for it to call (?)

I actually missed the fact that Channel.cond_take was actually a condition and that the method existed for GenericCondition. Made the change. I also removed unlock(f, ::GenericCondition) because after looking for a bit it's definitely a mistake and I added the missing function for WeakKeyDict.

@JeffBezanson
Copy link
Member

It looks like we are also missing lock(f, ::IO). That makes me think it's better just to remove all but one of the 2-argument lock methods, and define lock(f, l::Any).

@vtjnash vtjnash added the merge me PR is reviewed. Merge when all tests are passing label Apr 3, 2021
@vtjnash vtjnash merged commit d9f0f04 into JuliaLang:master Apr 6, 2021
@simeonschaub simeonschaub removed the merge me PR is reviewed. Merge when all tests are passing label Apr 28, 2021
ElOceanografo pushed a commit to ElOceanografo/julia that referenced this pull request May 4, 2021
Added lock(f, c::Channel) utility function
antoine-levitt pushed a commit to antoine-levitt/julia that referenced this pull request May 9, 2021
Added lock(f, c::Channel) utility function
johanmon pushed a commit to johanmon/julia that referenced this pull request Jul 5, 2021
Added lock(f, c::Channel) utility function
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.

5 participants