mxcl / PromiseKit

Promises for Swift & ObjC.
MIT License
14.23k stars 1.46k forks source link

Canceling a promise #18

Closed dbachrach closed 9 years ago

dbachrach commented 10 years ago

Consider an asynchronous HTTP GET method request that returns a promise:

- (Promise*)GET:(NSString*)path;

Users of this API may want to cancel the underlying asynchronous work, the HTTP request. Currently PromiseKit does not allow cancelable promises.

I've been looking into javascript promises and cancelation. There is some discussion in the A+ spec https://github.com/promises-aplus/cancellation-spec/issues

Any thoughts on cancelation?

mxcl commented 10 years ago

I have been thinking about it. I was wondering if the cancelation could be communicated via an attached NSProgress. This seems very objc.

NSURLConnection+PromiseKit would need to be rewritten to use the cancelable variety of NSURLConnection though (ie. delegation mechanism rather than completionBlock mechanism).

dbachrach commented 10 years ago

Interesting, I haven't used NSProgress yet. So you're suggesting:

Promise* myPromise = [obj GET:@"/"];
// .. later
[myPromise.progress cancel];

Makes sense. My question would be how much of the NSProgress API do you want to commit to. If it's just cancelation then having -cancel live on Promise would be clearer. If you want fractional progress indication and pausing too, then NSProgress looks right.

mxcl commented 10 years ago

Indeed, the reason I was thinking of NSProgress in the first place was because Promises also should have progress. So it seems appropriate. I haven't done much research yet though so am not 100% sure it will work right.

Thanks for the link, interesting and useful material.

Strilanc commented 10 years ago

You might also want to consider doing cancellation from the requester side, by passing in a cancel token that the async operation can hang off of. Often the thing that triggered the future is what knows when it should be cancelled. More reasoning here.

That's what I did in collapsing futures, anyways.

dbachrach commented 10 years ago

Here's a simple CancelToken, I've been using: https://gist.github.com/dbachrach/296586173a303c17c0ad

@mxcl I'm wondering if this is worth expanding upon and including in PromiseKit.

benjamingr commented 10 years ago

Note that promise cancellation is very problematic in the case two .thens are performed and then one of them cancels. It affects the other .then where it shouldn't.

csotiriou commented 10 years ago

I'm not trying to undermine the importance of the issue here, but I have some different thoughts on the matter. Although cancelling a promise would be nice to have, I feel that implementing a cancel action in a promise:

-- Will produce logical problems (see @benjamingr 's comment above) -- Will conflict with the cancellation logic of frameworks that have a cancellation logic on their own (e.i AFNetworking and the likes). -- It may produce more functional problems that it solves with multithreading, in case many promises depend on each other - what will a promise cause to a Promise chain, with some Promises in this chain being concurrent?

In most programming languages that have implemented a similar API (Javascript, for example), wrappers of functions (like Promises) don't actually handle cancellation on their own, if I am not mistaken. They keep things simple, for other developers to build their own frameworks on top of them, and have their own cancellation logic.

Asynchronous networking in all forms covers 90% of the cancellation needs of modern platforms. Asynchronous networking cancellation works like a breeze with PromiseKit and frameworks such as AFNetworking (shameless self-promotion : https://github.com/csotiriou/AFNetworking-PromiseKit ). You cancel an AFHTTPUrlRequestOperation, and the promise automatically stops. I have already implemented it into a high-profile project and it works flawlessly.

If you implement cancellation in a Promise, you must also provide a consistent API for frameworks supporting PromiseKit to integrate also their own cancellation logic. The rules (and limitations) for cancelling a Promise seem to be the same as the ones for cancelling an executing GCD block, which is a bit tricky, and not supported out of the box, for good reasons.

In the end, I am wondering, if it's worth all the effort, and if it will work (from a technical and logical standpoint) as we have it in our minds right now.

benjamingr commented 10 years ago

@csotiriou on 'other languages', as far as I can tell most languages do support cancellation - be it Tasks in C# or the cancellation spec in JS which most libraries support.

Kris had an interesting idea that if you make a promise cancellable only one subscriber may subscribe to it - that is: you can only .then it once.

dbachrach commented 10 years ago

@csotiriou In your library you have:

- (PMKPromise *)GET:(NSString *)URLString parameters:(id)parameters
{
    return [[self GET:URLString parameters:parameters success:^(AFHTTPRequestOperation *operation, id responseObject) {} failure:^(AFHTTPRequestOperation *operation, NSError *error) {}] promise];
}

How would I cancel that GET request? From a quick look through your code, I don't see how the caller of that method can cancel the task.

In my code I do this right now:

- (PMKPromise*)GET:(NSString*)URLString
    parameters:(NSDictionary*)parameters
    cancelToken:(MHCancelToken*)cancelToken
{
    return [PMKPromise new:^(PMKPromiseFulfiller fulfiller, PMKPromiseRejecter rejecter) {
          NSURLSessionDataTask* task = [self.sessionManager GET:URLString
                                                     parameters:parameters
                                                        success:^(NSURLSessionDataTask* resultTask, id responseObject) {
                                                            fulfiller(responseObject);
                                                        }
                                                        failure:^(NSURLSessionDataTask* resultTask, NSError* error) {
                                                            rejecter(error);
                                                        }];

          [cancelToken onCancel:^{
              if (task.state != NSURLSessionTaskStateCompleted) {
                  [task cancel];
              }
          }];
    }];
}

This was the main difficulty I ran into, which made me turn to cancelation tokens. If you've solved that without tokens, that would be great.

csotiriou commented 10 years ago

@benjamingr thank you for correcting me. However, I have a question, which may or may not be relevant to this thread: When you cancel a Task (or anything similar) is it certain that execution will actually stop, or that means that you give a "stop signal" and the promise may or may not stop according to the block being executed? Again, don't think only in terms of asynchronous networking, think in terms of other blocks, that may not have a cancellation logic by design (like a large image resizing algorithm).

@dbachrach I have indeed solved that issue, by cancelling the specific operation by looking at AFHTTPRequestOperationManager's operation queue. That means searching for the operation to cancel and identifying it using the URL and method. However, it works. Another logic is to create an AFHTPRequestOperation, keep it somewhere, and then call "-promise" on it and work as before. I use the first method, however.

After your answers, I believe I should stand corrected, and say that cancellation logic would be nice to have, if it's not always mandatory to use.

benjamingr commented 10 years ago

@csotiriou

First, in C# tasks do not represent asynchronous networking at all, they are much more often an abstraction over 'doing something' and are used as "easier to reason about" threads in many places. In JS it's mainly for concurrency although some people have implemented parallelism with it through web workers.

In C#, tasks are cancelled with a CancellationToken. Here is an example of how to use it.

Basically, it's more of a if (ct.IsCancellationRequested) - that is, like you said " the promise may or may not stop". Alternatively, it throws an exception in the async keyword case. You can handle it and recover but you're expected to cancel. In JavaScript a similar approach is taken.

In general, you can see how we handle it in JS here although we are not really satisfied with it.

Based on @KrisKowal 's idea. Here is a very basic adaptation I wrote which is just a suggestion that should be scrutinized before being used or even considered:

kriskowal commented 10 years ago

My position on cancellation has evolved. I am now convinced that cancellation is inherently impossible with the Promise abstraction because promises can multiple dependess and dependees can be introduced at any time. If any dependee cancels a promise, it would be able to interfere with future dependees. There are two ways to get around the problem. One is to introduce a separate cancellation "capability", perhaps passed as an argument. The other is to introduce a new abstraction, a perhaps thenable "Task", which in exchange for requiring that each task only have one observer (one then call, ever), can be canceled without fear of interference. Tasks would support a fork() method to create a new task, allowing another dependee to retain the task or postpone cancelation.

mxcl commented 10 years ago

In my mind it's possible. If we think of canceling as just making that promise rejected with an NSError with code PMKCanceled, then everything else works out automatically. Providing an override for new that is a pointer to a block that is called when the promise is canceled would allow the promise to react to cancelation if it can. If it can't then it will finish, but its value will be ignored.

Thening off a rejected promise does nothing, and this is how it already is. So the multiplicity factor is already handled.

I still would like to explore an NSProgress solution though.

kriskowal commented 10 years ago

Rejection from the producer is different than rejection from the consumer. Rejection from the consumer is a POLA violation, giving consumers the ability to interfere with each other’s progress. This manifests as the "Action-at-a-distance" antipattern. Since errors can flow both upstream and downstream, they can be broadcast laterally throughout a system. The reason for keeping the resolver and promise separate is to ensure that data flows in one direction, making programs robust and composable.

mxcl commented 10 years ago

Translating @kriskowal’s comment: a key-tenant of Promises is that other systems cannot mess with them, they reject or fulfill themselves, but only they have that power. Based on this knowledge you can right robust software. Adding a cancel method to a Promise would make it possible for any part of the chain to cancel other promises it has access to leading to unexpected behavior.

POLA is Principle of Least Astonishment. I find the acronym ironic.

I'm not totally sold on this argument. It's nice to imagine a world without astonishment but the real world involves cancelation.

However, ATEOD I don't think cancelation is a 80% use case. IMO it's not even a 95% use case. @kriskowal’s argument is quite compelling IMO for not adding features that aren't necessary that may lead to hard to debug systems.

kriskowal commented 10 years ago

POLA as in Authority in this case. And my argument is that cancellation does not work on promises, but could work on a different primitive mostly of the same shape and sharing the same principles.

mxcl commented 10 years ago

@kriskowal thanks for your input, I appreciate it.

albsala commented 10 years ago

I'm using [PMKPromise +when:] in order to synchronize two data download processes and I would find very useful to have a canceling option when one of these processes fails. In other case, I mean now, PMKPromise's behavior causes me problems.

mxcl commented 10 years ago

@albsala when automatically fails its promise when any of its array of input promises fail.

If you need to also cancel the remaining downloads then you will need to make your own promises for downloading that have a cancel mechanism. Wrapping a download is not particularly hard, just check NSURLConnection+PromiseKit.m.

I'll document how to implement a cancel mechanism at some point, but as said in depth above, we cannot easily add canceling to Promises without producing an API with unexpected behavior, so if canceling is absolutely essential for you then don't use promises or make your own promise that has a cancel capability.

kriskowal commented 10 years ago

Regarding cancelation in general [WIP] https://github.com/kriskowal/gtor/blob/master/cancelation.md#canceling-asynchronous-tasks

dbachrach commented 9 years ago

Just want to through out one more thing I've been thinking about. With collectionview/tableview cells that start asynchronous work, I could see it being useful to have a form of cancelation like so:

[ImageFetcher fetchImage:imageURL].ensure(^BOOL(){
    return /*whether this image request is still for this cell.*/;
}).then(^(UIImage* image) {
    cell.imageView.image = image;
});

ensure would continue then propagation if the result is YES. Otherwise, no thens or catches occur.

mxcl commented 9 years ago

The question for the framework is: is ensure clearer and more useful than an if statement in the then?

zdnk commented 9 years ago

ensure is better, but I would also appreciate cancelling the promise if I am doing some networking.

jkolb commented 9 years ago

@kriskowal Thank you, I had found that link somewhere but it was tricky to stumble onto when searching on the subject of promise cancellation. The last paragraph in it gave me inspiration.

In my promises library I've implemented cancellation by tying it to the lifetime of the Promise. When a promise is deinitialized the work it is doing can be cancelled early if not already finished. Only if all promises depending on another promise are deinitialized will it really cancel, otherwise it must stay alive and finish its work for the ones that are keeping it alive. Just because one section of the code cancelled doesn't mean other places in the code should be interrupted. The application programmer can then structure the promises to support early cancellation by making sure when needed they can be deinitialized at appropriate times.

To help support this, when you create a new promise you are provided with fulfill, reject, and isCancelled callbacks. Both fulfill and reject have weak references to the original promise so that if you call them and the promise has been deinitialized they safely have no effect. The isCancelled callback allows your long running background process to periodically call it to see if the owning promise has been deinitialized and exit early if it has. All promises in a chain have a reference to their parent promise and any promises returned within their callbacks, that way the lifetime of a promise is well defined. As long as the last promise returned from then is alive, then the entire chain is alive.

Any thoughts on if this is a viable strategy for implementing cancellation for PromiseKit?

mxcl commented 9 years ago

2.0 supports the idea of cancellation. Promises themselves cannot be cancelled, but the task they represent can (eg. [NSURLConnection cancel]). The promise then must ensure PromiseKit knows the specific NSError object that is generated is a cancellation error.

Cancellation errors skip to the next catch, but the catch handler is not executed unless it is called with catch(policy: .AllErrors).

This seemed the correct way to handle a cancellation. Not all promises are cancelable, promises that cancel do not resolve their chain. The catch handler usually is not interested in cancellation since either the user or the developer decided to end the chain explicitly, it is not strictly an error. Eg. with a typical flow with something that is cancellable:

UIApplication.sharedApplication().networkActivityIndicatorVisible = true
let cancellableThing = CancellableThing()
cancellableThing.go().then {
    //…
}.then {
    //…
}.finally {
    UIApplication.sharedApplication().networkActivityIndicatorVisible = false
}.catch { err in
    UIAlertView.show(err)
}

cancellableThing.cancel()

Here the finally will undo the UI changes but an error alert view will not be shown to the user. Here the developer decided to cancel the chain, presumably because they no longer care about the result. In such a situation neither an error should occur nor should the chain itself have to identify and handle that situation. Let's compare with what is necessary for an alert view in PromiseKit 1.x:

UIAlertView(…).promise().then { dismissedButtonIndex in
    if dismissedButtonIndex != alert.cancelButtonIndex {
        foo().then {
            // rightward drift
        }
    }
}

Rightward drift. Yuck.

Let me know if I should leave this open.

dbachrach commented 9 years ago

This all seems to make a lot of sense. I like the default policy of not raising cancelations to catch blocks, but do allow it via explicit indication. In the ensure() example we discussed previously, ensure could just be a generic extension to PromiseKit that would raise the cancelation NSError. I think that pattern needs to be vetted before inclusion in the library, though. I think this issue can be closed. Thanks Max.