Implement BFTask Cancellation

[danielrhammond]

Cancellation is provided by a token passed into the continuation methods, which check for the cancelled state before executing their blocks and return a cancelled BFTask if necessary, based on discussion from #18

Don't particularly like the multiple withs in the method signatures, but it seemed better to go follow the pattern of continueWithExecutor:withBlock/etc... really open to suggestions if you have something else you guys would prefer.

[danielrhammond]

One thing I was curious about and would love some help understanding for my own education is why BFTask creates a separate NSObject to use in its @synchronized blocks (the pattern I cargo-culted here). Is there a particular reason for doing this over just using self (as in @synchronized(self)) which seems like it would be less risky since its less likely to errantly be modified or released?

[josephearl]

Synchronizing around a private lock object is generally preferable to @synchronized(self) blocks unless you specifically want to share the lock with other objects which is what happens with self, since other objects can call @synchronized(you).

[josephearl]

The CancellationToken would better split into two classes like the C# API - CancellationToken and CancellationTokenSource.

[danielrhammond]

All BFTaskCancellationToken contains is a single atomic property tracking the the isCancelled state. This follows how NSOperation cancellation works and is based on the implementation design suggested by in #18

Whereas in the C# threading API I believe you're referring to [1] CancellationToken is responsible for propagating a notification on cancellation, monitoring whether task can be cancelled, and actually handling calling on registered delegates to react to the notification.

This is a very different design, and while interesting I struggle to see how it is relevant. Though I'd be happy to be shown where I'm incorrect, perhaps you can point out what part of the BFCancellationToken you think would be better served by splitting it out into a BFTaskCancellationToken and BFTaskCancellationTokenSource classes?

1 - Disclaimer: I am not a C# developer, but just looking at this doc

[josephearl]

@danielrhammond Apologies for not being very clear, I've listed the differences I think are worthwhile below.

Separation of CancellationToken and CancellationTokenSource

One difference is the separation of the cancel method and the isCancelled property by adding the CancellationTokenSource class. The purpose of this is that given a method such as:

- (BFTask) getStringAsync: (BFCancellationToken *) ct

The code inside the method should not have access to the cancel method of the token (it should use the TokenCompletionSource). Also the user code (that calls the method) has no need of the isCancelled property (it should use the property of the task).

All the logic can still stay on the BFCancellationToken itself with the BFCancellationTokenSource accessing hidden methods on the token much like BFTaskCompletionSource does with BFTask.

CancellationToken Listeners

Another is the use of listeners (the register) method to allow adaptation of existing async (e.g. callback-based) methods. Given an existing method such as:

- (Operation) getStringAsync: (CallbackBlock) cb

where Operation has a cancel method it is required to have some listener for the cancellation event in order to perform cancellation properly when adapting the method to a task. For example:

- (BFTask *) getStringAsyncTask: (BFCancellationToken *) ct
{
    BFTaskCompletionSource *tcs = ...;
    Operation *operation = getStringAsync: ^(NSString *result, NSError &error)
    {
        if (error != nil) {
            [tcs trySetCancelled];
        } else {
            [tcs trySetResult:result];
        }
    };
    [ct register: ^(BFCancellationToken *token)
    {
        [operation cancel];
        [tcs trySetCancelled];
    }];

    return tcs.task;
}

[soleares]

@danielrhammond Ideally the multiple withs could be removed from the existing API since IMHO they don't follow Cocoa method naming conventions.

[danielrhammond]

While I agree @soleares its not very good looking cocoa (hence me calling it out in the initial description), that seems to be a change outside of the scope of this pull-request and should be considered carefully as a separate change especially given that it would break the public API.

[danielrhammond]

Just pushed up a rebase to fix the broken github auto-merge in case that helps provide some impetus for one of the maintainers to take a look. 🍻

[nlutsenko]

This got superseeded by #89
If there is anything there that you think is relevant - please send a PR!

[facebook-github-bot]

@danielrhammond updated the pull request.