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

Fix potential spurious thread wakeup. #247

Merged
merged 1 commit into from
Apr 15, 2016
Merged

Fix potential spurious thread wakeup. #247

merged 1 commit into from
Apr 15, 2016

Conversation

nlutsenko
Copy link
Member

Getter for self.completed is protected with a @synchronized call, so this should be thread-safe to call like that in a loop.

Superseds #135 and fixes #134.

@nlutsenko nlutsenko added this to the 1.8.0 milestone Apr 12, 2016
@nlutsenko
Copy link
Member Author

@toddreed will be curious to hear your thoughts on this one, since you are the author of the original PR.
I even back ported the test from that PR.

@toddreed
Copy link

I think this will work. My only quibble (and perhaps just my own personal stylistic bias) is the mixing of @synchronized and NSCondition to achieve thread synchronization for the concept of "completion". My understanding of NSCondition is that it is meant to provide both mutual exclusion (lock/unlock) and thread synchronization (wait/signal). I think the common usage pattern for NSCondition is for it to represent a boolean condition, and the state representing that condition is protected by the NSCondition. So, in this case, the condition is "completed", backed by the state variable _completed, and access to this state variable should thus be protected by the NSCondition (i.e. lock/unlock). But, the NSCondition is really only being used for the wait/signal functionality and @synchronized is being use for the mutual exclusion; this is the part I consider unconventional usage. I prefer my last commit for #135 because I think it's more explicit about the relationship between the NSCondition and the variable _completed and follows (what I believe to be) the conventional usage pattern for NSCondition.

All that said, the commit from #135 introduces more changes, and with my limited familiarity of the code I think your change is simpler and safer.

@nlutsenko
Copy link
Member Author

Awesome @toddreed, I agree with you. The only concern that I would have about your model is that using NSCondition as a lock is probably not great, because it's not recursive, where we explicitly need one, as we allow getting properties of a task from inside it's continuation. Thus usage of @synchronized, which is fully recursive works good for us.
This is indeed a simpler and safer change for #134, but I am open to ideas on how we could tackle usage of a single lock for both waiting and synchronizing access.

@nlutsenko nlutsenko merged commit d326aa0 into master Apr 15, 2016
@nlutsenko nlutsenko deleted the nlutsenko.wakeup branch April 15, 2016 01:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

-[BFTask waitUntilFinished] does not account for spurious thread wakeup
4 participants