[iOS] Returning operations from sync actions #709
Conversation
|
Hi @damienpontifex, I'm your friendly neighborhood Azure Pull Request Bot (You can call me AZPRBOT). Thanks for your contribution! The agreement was validated by Microsoft and real humans are currently evaluating your PR. TTYL, AZPRBOT; |
damienpontifex
changed the title
| completion:completion]; | ||
|
|
||
| // Before we can pull from the remote, we need to make sure out table doesn't having pending operations | ||
| NSArray *tableOps = [self.operationQueue getOperationsForTable:query.table.name item:nil]; |
There was a problem hiding this comment.
Keeping this check on the original queue here will incur a performance penalty in the user's app, as now they are waiting for a db read. What's the rationale for keeping it here?
There was a problem hiding this comment.
Originally I didn't see a way to return the pull operation from an async queue. Then I moved the pull constructor up to the first expression and I see I can add the async call back in. Will do it.
|
I expected to see an update to sdk/iOS/src/WindowsAzureMobileServices.h to list the Queue*Operation files, as well as adding the {ATTRIBUTES = (Public, ) option in the pbxproj file. |
|
@shrishrirang that seems like a good solution. I have tested that and seems to work as well. |
…o keep operation subclasses private
| @@ -8,6 +8,7 @@ | |||
|
|
|||
| @class MSQuery; | |||
| @class MSSyncContext; | |||
| @class MSQueuePushOperation; | |||
There was a problem hiding this comment.
Is this change needed now (in this file and other files)?
| // For now we just abort the pull if the push failed to complete successfully | ||
| // Long term we can be smarter and check if our table succeeded | ||
| if (error) { | ||
| [pull cancel]; |
There was a problem hiding this comment.
Are you sure calling cancel is enough? Doesn't that just set the isCancelled flag? The op itself wouldn't set isFinished until it goes to run. What flag is the dependent Op observing?
There was a problem hiding this comment.
Cancelling the NSOperation would mark it finished, so the observing task should be fine.
There was a problem hiding this comment.
The operation wouldn't have even started at this point, so I don't think we need to do anything else. The completion block will be called if the user has provided one to notify them of the failure.
I could do some testing though with the returned pull operation as a dependency of another operation. If the execution flows through to the waiting operation when this is cancelled, should all be fine.
There was a problem hiding this comment.
Can you link to docs? Everything I see online, says that the behavior of cancel isn't different if the operation is not running. So if you want to trigger a dependent task, you are responsible for setting the isFinished flag as well.
There was a problem hiding this comment.
Running through a unit test, you were right @phvannor that without explicitly setting the finished status, the dependent tasks wouldn't be completed.
Latest commit with changes to access internal method for completion plus unit test for this case added.
There was a problem hiding this comment.
Thanks for adding UT to test this path.
Btw, should we not simply add the pull NSOperation to the queue immediately after making it dependent on the push NSOperation (instead of waiting for the push to complete successfully)? That way you won't need to call 'completeOperation' explicitly. Just calling cancel ( [push cancel] ) will be enough.
There was a problem hiding this comment.
Sadly, then the pull would possibly already be running, so we'd actually need the push op to expose its error state, so the pull op could look at its dependencies & verify they are were ok before running. Probably worth looking into, but not sure on effort to do so vs what we have now.
[iOS] Returning operations from sync actions
Closes #703 by bringing these changes into dev branch first.
Returns NSOperation types from sync operations to allow the use to use them to react to changes. This could be to cancel, or to add these as dependencies to a trailing operation.