Implement sync_ barrier based concurrency management #303
Conversation
|
@flovilmart, this is a good approach, though I would recommend looking at how we dealt with task state in Bolts-Swift and potentially porting it over here. I will give it a tad more time to review, since it’s crucial to make sure that we’ll never get a deadlock, where we might have cases not covered by tests right now. Also cc @richardjrossiii |
|
Yeah, I encountered a few deadlocks while developing it. I’ll check out the Swift implementation for the state management, and improve. |
|
@nlutsenko I introduced the BFTaskState, not sure if that's what you had in mind. |
|
@nlutsenko We'll release our future QA version with this custom build, it all look good so far, no more thread sanitizer alert, no deadlocks. |
Bolts/Common/BFTask.m
Outdated
| return _result; | ||
| } | ||
| __block id result; | ||
| dispatch_barrier_sync(_synchronizationQueue, ^{ |
There was a problem hiding this comment.
You only need to use barrier_sync for writes - for reads, just use dispatch_sync. You'll get read-write lock behavior here, with a slight perf improvement.
Bolts/Common/BFTask.m
Outdated
| @@ -16,6 +16,13 @@ | |||
|
|
|||
| NS_ASSUME_NONNULL_BEGIN | |||
|
|
|||
| typedef enum : NSUInteger { | |||
| pending, | |||
There was a problem hiding this comment.
Let's use more descriptive names here. BFTaskStatePending, BFTaskStateSucceeded, etc.
Bolts/Common/BFTask.m
Outdated
| [self.callbacks removeAllObjects]; | ||
| }); | ||
| for (void (^callback)() in callbacks) { |
There was a problem hiding this comment.
There was a reason IIRC that we had the callbacks executing in the context of self.lock before. I believe it had to do with a race whereupon you could enqueue a new callback in-between -removeAllObjects and execution of the callbacks, and allow another continuation to execute out-of-order.
If I recall from the conversations we had at the time, we weren't comfortable changing those guarantees for people who were using Bolts already, whereas Bolts-Swift was able to break those guarantees as they didn't exist yet.
I'm OK with switching it to this version, I just wanted to make sure the context is documented here.
There was a problem hiding this comment.
OK, I recall I had issues with keeping the execution in the sync block, perhaps outside a barrier is better, LMK.
There was a problem hiding this comment.
@richardjrossiii this seems to lead to a deadlock in that test
when processing the continuations inside the block
Bolts/Common/BFTask.m
Outdated
| BOOL completed = self.isCompleted; | ||
| dispatch_barrier_sync(_synchronizationQueue, ^{ | ||
| if (!completed) { | ||
| [self.condition lock]; |
There was a problem hiding this comment.
This is technically undefined behavior here. There's no guarantee on the thread that synchronizationQueue runs on, and NSConditionLock requires you to unlock from an identical thread. We may need to rethink this. (What's the bolts-swift way of doing this?)
There was a problem hiding this comment.
seems very similar: https://github.com/BoltsFramework/Bolts-Swift/blob/master/Sources/BoltsSwift/Task.swift#L211
main difference is that the NSConditionLock is created set on the queue, let me know if you prefer that way.
Bolts/Common/BFTask.m
Outdated
| if (self.completed) { | ||
| return; | ||
| BOOL completed = self.isCompleted; | ||
| dispatch_barrier_sync(_synchronizationQueue, ^{ |
There was a problem hiding this comment.
We're locking twice here by going through the property (use ivar instead?). Theres also a TOCTOU race condition here.
Bolts/Common/BFTask.m
Outdated
| __block NSString *resultDescription = nil; | ||
|
|
||
| dispatch_barrier_sync(_synchronizationQueue, ^{ | ||
| isCompleted = _state != pending; |
There was a problem hiding this comment.
Just take the state, result pair out, don't do any additional work inside the lock. Acquire the lock for as little time as is safely possible.
|
@richardjrossiii I believe I addressed your suggestions let me know if there's anything else. I know this is a tricky part with concurrency and I'm a bit rusted on those problematics. |
|
@richardjrossiii @nlutsenko the Parse SDK is all fine with that version and the thread sanitizer is happy for what I can tell. See: https://travis-ci.org/parse-community/Parse-SDK-iOS-OSX/builds/273015827 |
|
@richardjrossiii @nlutsenko a quick ping on that one, I'm open to re-visiting another option that would let Xcode not warn about potential unsafe memory accesses. |
|
@nlutsenko ping? |
|
@nlutsenko any time for the review? |
|
I'm a bit out of my depth here. Can anyone can confirm this issue has potential to cause instability? I.e. would solving the problem actually make Bolts more stable, or is this mainly to silence Xcode's thread sanitizer tool? @nlutsenko @flovilmart |
This reverts commit ed7fbb8. This yields deadlocks when testing against the parse SDK
Addresses issue #302
Also implements BoltsSwift resource protection through a queue and
barrier_synccc @nlutsenko as you were marked in the TODO.