Task Cancellation

[josephearl]

Implement task cancellation as per Task Cancellation.

• Add CancellationToken and CancellationTokenSource to match system.threading.cancellationtoken and system.threading.cancellationtokensource
• Add methods to Task that accept CancellationTokens
• Add AppLinkResolver method that accepts CancellationTokens

[josephearl]

extra newline

[grantland]

Let's revert the changes in AppLinkResolver.java and WebViewAppLinkResolver.java. Adding a method to AppLinkResolver is a breaking change since it requires developers to implement an extra method and it doesn't seem like we're utilizing the CT version of the method in WebViewAppLinkResolver anyway.

I'm not sure how I feel about CancellationListener. I understand you added it to wrap other CancellationToken types from other APIs, but it would be great if we can move it to another PR to discuss.

Otherwise, everything is looking good! You've been making awesome contributions :)

[josephearl]

@grantland updated with suggested changes, will move the CancellationListener into another PR

RE AppLinkResolver changes - yes, I forgot to also make the changes to AppLinkNavigation.
Perhaps we could create a second AppLinkResolver2 (or CancellableAppLinkResolver) interface and add extra methods to AppLinkNavigation to use that together with CancellationToken.

[grantland]

{@link CancellationTokenSource}

{@link CancellationTokenSource#getToken()}

{@link CancellationTokenSource#cancel()}

[grantland]

@josephearl Thanks! I've added some doc changes and I'm planning on doing another once-over for the API and implementation, so you can wait on updating.

RE AppLinkResolver - I'm not sure AppLinkResolver2 would work since we'd need to accept the base implementation in AppLinkNavigation#navigateInBackground, which would also be a breaking change. Are you using AppLinkResolver in your codebase?

[josephearl]

@grantland I think there would need to be a new AppLinkNavigation#navigateInBackground method which accepts a CancellationToken in addition to the resolver, so this method would accept AppLinkResolver2 rather than changing the current method.

AppLinkResolver2 would extend AppLinkResolver to remain compatible with the existing methods.

[grantland]

Re AppLinkResolver2: That might be a good solution, but unfortunately I'll have to defer AppLinks discussions to @mingflifb. It'll probably be better in a separate PR.

I also just noticed CancellationToken.Register(Action), which looks be similar to the CancellationListener you added. I'm sorry I missed this, but feel free to add that back to this PR if you'd like it to be reviewed again.

Please note that the following isn't about requesting things to be changed, but opening discussion:

TPL defines FromCanceled<TResult>(CancellationToken). Do we want to support something similar? We already have <TResult> Task<TResult> cancelled() and looking at the TPL source code, FromCanceled throws if the token isn't actually cancelled, which brings me to believe that it needs to be wrapped with an if (ct.isCancellationRequested) anyway. If this is the case, cancelled() seems like it should suffice.

Do we want to add CancellationToken#none() and CancellationToken#canBeCanceled()?

Otherwise, the implementation looks great!

[josephearl]

OK I'll re-add CancellationToken.register(). On issue in comparison with the C# implementation is a need to unregister to avoid leaking memory. Consider something like the following:

Task<String> getStringAsync(CancellationToken ct) {
    TaskCompletetionSource<String> tcs = new Task.create();
    final Operation<String> operation = getStringOperationAsync(new Callback<String>() {
        void onComplete(String response) {
            tcs.setResult(response);
        }
     }
     ct.register(new CancellationListener() {
         void onCancellationRequested(CancellationToken ct) {
              operation.cancel();
              tcs.trySetCancelled();
         }
     });
}

If the operation completes successfully, the listener is still registered and retaining operation (and the same token may be shared between a number of tasks).

Also the cancellation token may be cancelled some time later after this operation has completed. In this case the code above still attempts to cancel the operation.

To avoid this an unregister method is needed.

And since the CancellationListener will be registered (and thus created) after the operation has been created there needs to be some way of identifying the listener when registering/unregistering independent of the listener object itself.

I suppose another option to the ID could be to Capture the cancellation listener so the Capture can be created before the operation.

Task.fromCancelled looks like a sensible addition so I'll add that. What kind of exception do you think is appropriate, IllegalStateException?

Just to be clear on CancellationToken#none() and CancellationToken#canBeCanceled() - CancellationToken#none() returns a token that cannot be cancelled, whereas the token from CancellationTokenSource can be cancelled?

[grantland]

Re CanBeCanceled/None:
to be honest, I'm not very experienced with these APIs, but from what I can tell from docs/source you're correct. it also doesn't seem to be 100% necessary atm, so feel free to pass on that.

Re Adding fromCanceled(CancellationToken):
my original comment was just for leaving a trail of our decisions, do you really think it's sensible to add? I'm just wondering when we would actually use it vs cancelled()

Also, let me read into register really quick. It seems there's a bunch of complexity I didn't anticipate.

[grantland]

We actually might have enough for one PR. Let's just clean up the last nits and move the other topics to their own PRs, since they don't really have any impact on any APIs or implementations from this PR. How does that sound?

[josephearl]

@grantland sure.

I'll update the documentation add Task.fromCancelled(). I think it makes sense since it doesn't require adding any features to CancellationToken and matches fromResult and the C# API. Not entirely sure on the use-case.

[josephearl]

Looks like in C# it is the only way to create a cancelled task, so actually I don't think we need it given we already have cancelled().

[mingflifb]

I think adding a CancellableAppLinkResolver makes sense.

[grantland]

@josephearl: To clarify, we're not adding Task.fromCancelled(CancellationToken) since we have Task.cancelled(), right?

[josephearl]

@grantland yes I've left that out. Updated.

[grantland]

{@link CancellationTokenSource#getToken()}

[grantland]

@see CancellationTokenSource
@see CancellationTokenSource#getToken()
@see CancellationTokenSource#cancel()

[grantland]

Implementation looks good, just a few doc nits. I'll also assume you're adding CancellationToken.register() in another PR, since it's not in this one.

Thanks!

[josephearl]

@grantland Updated docs as per your comments.

Yes I will move CancellationToken.register() to another PR - I'd rather get this approved and submit a matching PR for iOS first :)

[grantland]

Looks great, thanks! I'll look forward to the rest of it :)