Proposal for PromiseKit cancellation support #896
Comments
|
I've create the pull requests for packaging options 1 and 2. I see that some of the Travis builds are failing for the pull requests, and am working on fixes now. |
|
Thanks for this, I appreciate the work and I think you have good direction. I will take some time, maybe a lot, since this is a lot to review. I'm confident we can work this into the framework, but I am very cautious about things with this project: there are many alternatives to PromiseKit, and I feel the reason PMK is popular is I am careful to keep it high quality. Anyway, I will review. |
|
Awesome! Yes, I totally understand about keeping PromiseKit concise and high quality. I'm really up for any of the 3 options or anything else you have in mind. And yes, it is a lot so no need to rush anything -- better to take the time and make great decisions. My preference above is based purely on how easy it is for people to use the new cancellation feature, and putting myself in your shoes I see that stability and maintaining high quality are high priority concerns. A nice upside with this delegate approach is it can be integrated or decoupled as desired! The Travis builds for all the cancellation pull requests are passing, with the exception of some Swift Package Manager related issues in the Foundation extension build. Will see if I can get those green as well. I would say it is ready for review! |
|
One thing that concerns me is the The question is, if it is really needed to touch all |
|
Responding to Zdeněk's excellent questions -- CC suffixI agree, I don't much like the 'CC' suffix. But I failed to come up with something better. I discovered that there needs to be something different about the signature for all methods that create a cancellable promise or cancellable guarantee, otherwise you run into many ambiguous situations that would both break existing code and make writing new cancellable code tedious. For example without the 'CC' suffix or some equivalent, code like the following gives 'ambiguous' compilation errors: To make it work you'd need to explicitly specify the types. For regular promises this is not an option because it would break existing code -- And for cancellable promises it would be quite tedious -- Originally I went with 'cancellable' as a prefix rather than the 'CC' suffix, but 'cancellable' just felt too long to put everywhere. I also tried using a new required paramter but that looked ugly as well. So I settled on 'CC', which I supposed could be considered a very abbreviated version of 'Cancellable'. Therefore 'dataTaskCC' is an abbreviation for 'dataTaskCancellable', meaning the cancellable varient of dataTask. Would 'WithCancel' abbreviated 'WCL' or 'WCC' be better? For example 'dataTaskWithCancel' abbreviated to 'dataTaskWCL'? Or a longer abbreviation for cancellable like 'CNBL'? Well, that last one sounds more like cannibal. Hmmmm. I'm open to ideas on this one!! The problem could be completely avoided by baking cancellation directly into Promise and Guarantee (i.e. a new 'cancel' method on those classes), but this would make everything cancellable which has its own drawbacks. Btw, one of the risks of the new cancellation code is breaking existing code with 'ambiguous' compilation errors. I wrote a bunch of tests to alleviate this risk and hopefully caught all the cases. I got some of these errors where I really thought it should be unambiguous, but the current compiler seems rather tempermental in this particular area. Why touch all promises?I'm understanding this question as, why have cancellable versions for Thenable, Guaratee, after, firstly, hang, race and when? The reason is simply, I want to provide cancellation abilities for the entire PromiseKit API. Done this way, you can add cancellation abilities to any existing code simply by adding the 'CC' suffix (or some alternative) to the methods that are actually creating the promises. The above example -- becomes cancellable with minor changes: Thank you for the feedback!! |
|
Hello, thanks for extensive response! Well, the thing I do not understand why you would need 2 versions of I believe then On rule: Non-cancellable tasks return Let me know your thoughts :) |
|
Hi guys, I'm not a developer of PromiseKit, I just use it. I'm struggling a bit to understand the use cases this solves and I'm presuming there are two:
Am I correct in that scenario #2 is the one being addressed by this PR? The idea of not having to add guard blocks to effectively monitor external state in order to cancel a chain sounds great. But I'm not keep on expanding the range of functions like the above. If feels like it's creating a large amount of complexity which will make life harder for both experienced and newbie PromiseKit user's alike. I'm especially keen on this because one of the things I love about PromiseKit is it's simplicity. I have another idea which just jumped into my head whilst writing this :-) so I don't know if it's practical at all. Basically it's to make Not sure if that's workable, just throwing around some ideas in my head. |
|
Well I like the idea of having a chain member like your |
|
Yes, agreed -- this proposal is for cancelling promises externally. And, the idea is that this can be done with very minimal code changes. I agree with Zdeněk's image loading example where you just want to cancel the whole operation. Another example is a user typing with search completions -- with each keystroke, you want to cancel the previous search (of course you'd introduce a small delay to reduce the number of search operations which you can conveniently do with 'after', but I digress). In fact, the only code change necessary is to specify that you want the 'cancellable' option. This is what all the 'CC' methods are for, they are saying that you want the chain to be cancellable. A different approach would be to use a flag or parameter instead of the 'CC' suffix. You just need something there to say you want cancellation ability. The 'cancelIf' could work well, but I'm trying to avoid requiring the developer to add new code for handling cancellation. I think it is quite nice to just call 'cancel', and it just stops executing without executing any more code blocks. If you need to add some cancellation handling code you can do it in the 'catch' with the '.allErrors' policy. OR, it is possible to simply default to having everything be 'cancellable'. Then there would be no need to specify a 'cancellable' option and the 'CC' methods would go away. The 'cancel()' method would be defined right inside 'Promise' rather than 'CancellablePromise'. Perhaps for completeness it would be good exercise to create a pull request for 'Option Zero', where everything defaults to be cancellable. There would be no 'CC' methods and 'cancel()' would be defined directly inside 'Promise'. I've been proceeding on the assumption that cancellation should be optional, but perhaps this comes at the cost of a less intuitive API. I realize there are some big downsides to having cancellation as a default. But 'Option Zero' could be useful for comparison at least. I'll give it a go. The to summarize the options (as defined in the Packaging section above):
|
|
Yeah, the Please excuse me if I'm suggesting what you've already done. I really haven't had the time to research this like you guys have. I presume the builtin |
|
Regarding The builtin |
|
I would like to remove cancelation from all extensions that are not inherently cancelable, eg: Alamofire is inherently cancelable, the networking can be canceled.
However I understand that sometimes you want to just ignore something like an This removes a lot of the additional API in the extensions and reduces the overwhelmingness of someone reading The API significantly. I still have concerns, I fear that in practice the burden on people constructing APIs and chains with promises will increase significantly due to this change. Because I think it is important that in general users are encouraged to not offer cancellation in situations where it would cause them to leak methods that allow the caller to break the state machine of the callee. So in general it should be opt-in to return a cancellation context. But we can see how it is in a 7-alpha release. The cc suffix can stay for now, but certainly we will lose it for a final release. I feel |
|
This is great! I really like your suggestion for a Having cancelation as an opt-in feature sounds great to me! A couple questions: What is the best way to eliminate the
Btw I've been spelling 'cancellable' with two Ls to match the existing |
|
This is pretty much ready to go, pending additional review. Docs and code are all in shape to be reviewed now. The 'Documentation/Cancel.md' doc is much clearer now. 'CC' methods are gone and 'CancellableGuarantee' is gone. Everything is passing in the CI reports. More details are available in the pull request: #899 |
I've updated the 'Cancel' doc to match the latest code, please feel free to provide feedback!! Here is the latest (nearly finalized?) version:Cancelling PromisesPromiseKit 7 adds clear and concise cancellation abilities to promises and to the PromiseKit extensions. Cancelling promises and their associated tasks is now simple and straightforward. Promises and promise chains can safely and efficiently be cancelled from any thread at any time. UIApplication.shared.isNetworkActivityIndicatorVisible = true
let fetchImage = cancellable(URLSession.shared.dataTask(.promise, with: url)).compactMap{ UIImage(data: $0.data) }
let fetchLocation = cancellable(CLLocationManager.requestLocation()).lastValue
let promise = firstly {
when(fulfilled: fetchImage, fetchLocation)
}.done { image, location in
self.imageView.image = image
self.label.text = "\(location)"
}.ensure {
UIApplication.shared.isNetworkActivityIndicatorVisible = false
}.catch(policy: .allErrors) { error in
/* 'catch' will be invoked with 'PMKError.cancelled' when cancel is called on the context.
Use the default policy of '.allErrorsExceptCancellation' to ignore cancellation errors. */
self.show(UIAlertController(for: error), sender: self)
}
//…
// Cancel currently active tasks and reject all cancellable promises with 'PMKError.cancelled'.
// 'cancel()' can be called from any thread at any time.
promise.cancel()
/* 'promise' here refers to the last promise in the chain. Calling 'cancel' on
any promise in the chain cancels the entire chain. Therefore cancelling the
last promise in the chain cancels everything. */Cancel ChainsPromises can be cancelled using a Creating a chain where the entire chain can be cancelleed is the recommended usage for cancellable promises. The For example: let context = firstly {
/* The 'cancellable' function initiates a cancellable promise chain by
returning a 'CancellablePromise'. */
cancellable(login())
}.then { creds in
cancellable(fetch(avatar: creds.user))
}.done { image in
self.imageView = image
}.catch(policy: .allErrors) { error in
if error.isCancelled {
// the chain has been cancelled!
}
}.cancelContext
// …
/* Note: Promises can be cancelled using the 'cancel()' method on the 'CancellablePromise'.
However, it may be desirable to hold on to the 'CancelContext' directly rather than a
promise so that the promise can be deallocated by ARC when it is resolved. */
context.cancel()Creating a partially cancellable chainA Convert a cancellable chain to a standard chain
/* Here, by calling 'promise.then' rather than 'then' the chain is converted from a cancellable
promise chain to a standard promise chain. In this example, calling 'cancel()' during 'login'
will cancel the chain but calling 'cancel()' during the 'fetch' operation will have no effect: */
let cancellablePromise = firstly {
promise = cancellable(login())
}
cancellablePromise.promise.then {
fetch(avatar: creds.user)
}.done { image in
self.imageView = image
}.catch(policy: .allErrors) { error in
if error.isCancelled {
// the chain has been cancelled!
}
}
// …
/* This will cancel the 'login' but will not cancel the 'fetch'. So whether or not the
chain is cancelled depends on how far the chain has progressed. */
cancellablePromise.cancel()Convert a standard chain to a cancellable chain A non-cancellable chain can be converted to a cancellable chain in the middle of the chain as follows: /* In this example, calling 'cancel()' during 'login' will not cancel the login. However,
the chain will be cancelled immediately, and the 'fetch' will not be executed. If 'cancel()'
is called during the 'fetch' then both the 'fetch' itself and the promise chain will be
cancelled immediately. */
let promise = cancellable(firstly {
login()
}).then {
cancellable(fetch(avatar: creds.user))
}.done { image in
self.imageView = image
}.catch(policy: .allErrors) { error in
if error.isCancelled {
// the chain has been cancelled!
}
}
// …
promise.cancel()TroubleshootingAt the time of this writing, the swift compiler error messages are usually misleading if there is a compile-time error in a cancellable promise chain. Here are a few examples where the compiler error is not helpful. Cancellable promise embedded in the middle of a standard promise chain Error: Ambiguous reference to member let promise = firstly { /// <-- ERROR: Ambiguous reference to member 'firstly(execute:)'
/* The 'cancellable' function initiates a cancellable promise chain by
returning a 'CancellablePromise'. */
login() /// SHOULD BE: "cancellable(login())"
}.then { creds in
cancellable(fetch(avatar: creds.user))
}.done { image in
self.imageView = image
}.catch(policy: .allErrors) { error in
if error.isCancelled {
// the chain has been cancelled!
}
}
// ...
promise.cancel()The return type for a multi-line closure returning The Swift compiler cannot (yet) determine the return type of a multi-line closure. The following example gives the unhelpful error: Enum element let promise = firstly {
cancellable(login())
}.then { creds in /// SHOULD BE: "}.then { creds -> CancellablePromise<UIImage> in"
let f = fetch(avatar: creds.user)
return cancellable(f)
}.done { image in
self.imageView = image
}.catch(policy: .allErrors) { error in /// <-- ERROR: Enum element 'allErrors' cannot be referenced as an instance member
if error.isCancelled {
// the chain has been cancelled!
}
}
// ...
promise.cancel()Declaring a You'll get a very misleading error message if you declare a return type of let promise = firstly { /// <-- ERROR: Ambiguous reference to member 'firstly(execute:)'
/* The 'cancellable' function initiates a cancellable promise chain by
returning a 'CancellablePromise'. */
cancellable(login())
}.then { creds -> Promise<UIImage> in /// SHOULD BE: "}.then { creds -> CancellablePromise<UIImage> in"
let f = fetch(avatar: creds.user)
return cancellable(f)
}.done { image in
self.imageView = image
}.catch(policy: .allErrors) { error in
if error.isCancelled {
// the chain has been cancelled!
}
}
// ...
promise.cancel()Trying to cancel a standard promise chain Error: Value of type let promise = firstly {
login() /// SHOULD BE: "cancellable(login())"
}.then { creds in
fetch(avatar: creds.user) /// SHOULD BE: cancellable(fetch(avatar: creds.user))
}.done { image in
self.imageView = image
}.catch(policy: .allErrors) { error in
if error.isCancelled {
// the chain has been cancelled!
}
}
// ...
promise.cancel() /// <-- ERROR: Value of type 'PMKFinalizer' has no member 'cancel'Core Cancellable PromiseKit APIThe following classes, methods and functions have been added to PromiseKit to support cancellation. Existing functions or methods with underlying tasks that can be cancelled are indicated by being wrapped with 'cancellable()'. ExtensionsCancellation support has been added to the PromiseKit extensions, but only where the underlying asynchronous tasks can be cancelled. This example Podfile lists the PromiseKit extensions that support cancellation along with a usage example: Here is a complete list of PromiseKit extension methods that support cancellation: Choose Your Networking LibraryAll the networking library extensions supported by PromiseKit are now simple to cancel! // pod 'PromiseKit/Alamofire'
// # https://github.com/PromiseKit/Alamofire
let context = firstly {
cancellable(Alamofire
.request("http://example.com", method: .post, parameters: params)
.responseDecodable(Foo.self))
}.done { foo in
//…
}.catch { error in
//…
}.cancelContext
//…
context.cancel()And (of course) plain // pod 'PromiseKit/Foundation'
// # https://github.com/PromiseKit/Foundation
let context = firstly {
cancellable(URLSession.shared.dataTask(.promise, with: try makeUrlRequest()))
}.map {
try JSONDecoder().decode(Foo.self, with: $0.data)
}.done { foo in
//…
}.catch { error in
//…
}.cancelContext
//…
context.cancel()
func makeUrlRequest() throws -> URLRequest {
var rq = URLRequest(url: url)
rq.httpMethod = "POST"
rq.addValue("application/json", forHTTPHeaderField: "Content-Type")
rq.addValue("application/json", forHTTPHeaderField: "Accept")
rq.httpBody = try JSONSerialization.jsonData(with: obj)
return rq
}Cancellability Goals
let promise = firstly {
cancellable(login()) // Use the 'cancellable' function to initiate a cancellable promise chain
}.then { creds in
fetch(avatar: creds.user)
}.done { image in
self.imageView = image
}.catch(policy: .allErrors) { error in
if error.isCancelled {
// the chain has been cancelled!
}
}
//…
promise.cancel()
import Alamofire
import PromiseKit
func updateWeather(forCity searchName: String) {
refreshButton.startAnimating()
let context = firstly {
cancellable(getForecast(forCity: searchName))
}.done { response in
updateUI(forecast: response)
}.ensure {
refreshButton.stopAnimating()
}.catch { error in
// Cancellation errors are ignored by default
showAlert(error: error)
}.cancelContext
//…
/* **** Cancels EVERYTHING (except... the 'ensure' block always executes regardless)
Note: non-cancellable tasks cannot be interrupted. For example: if 'cancel()' is
called in the middle of 'updateUI()' then the chain will immediately be rejected,
however the 'updateUI' call will complete normally because it is not cancellable.
Its return value (if any) will be discarded. */
context.cancel()
}
func getForecast(forCity name: String) -> CancellablePromise<WeatherInfo> {
return firstly {
cancellable(Alamofire.request("https://autocomplete.weather.com/\(name)")
.responseDecodable(AutoCompleteCity.self))
}.then { city in
cancellable(Alamofire.request("https://forecast.weather.com/\(city.name)")
.responseDecodable(WeatherResponse.self))
}.map { response in
format(response)
}
} |
|
Merged and tagged 7.0.0-alpha1, available for testing! |
|
How do I cancel the task wrapped within the promise with the new APIs? Like an Operation or network request? |
|
Well, after lots of code reading it seems to me that the key is to subclass Cancellable and wrap the underlying task. Then cancel it on cancel(). Pass the Cancellable to the CancellablePromise and when the promise is cancelled, the Cancellable will get cancelled as well. |
|
Sorry for the confusion -- there's a method on Promise called 'setCancellableTask' for this purpose. You can set the cancellable task on the regular Promise and then use the 'cancellize' function to convert it to a CancellablePromise. The cancellable extensions have not yet been included in PromiseKit V7, so I can see how this is confusing. For example, the URLSession.dataTask in PMKFoundation is made cancellable as follows: extension URLSession {
public func dataTask(_: PMKNamespacer, with convertible: URLRequestConvertible) -> Promise<(data: Data, response: URLResponse)> {
var task: URLSessionTask!
var reject: ((Error) -> Void)!
let promise = Promise<(data: Data, response: URLResponse)> {
reject = $0.reject
task = self.dataTask(with: convertible.pmkRequest, completionHandler: adapter($0))
task.resume()
}
promise.setCancellableTask(task, reject: reject)
return promise
}
}
...
class MyRequest {
func performCancellableURLRequest() {
let request = URLRequest(url: URL(string: "http://example.com")!)
let promise = firstly {
cancellize(URLSession.shared.dataTask(.promise, with: request))
}.compactMap {
try JSONSerialization.jsonObject(with: $0.data) as? NSDictionary
}.done { rsp in
...
}.catch(policy: .allErrors) { error in
error.isCancelled ? print("cancelled") : print("Error: \(error)")
}
...
// To cancel:
promise.cancel()
}
} |
|
Hey, I saw this code in the PMKFoundation but it didn't make sense initially, but now I get it. |
|
Ah! I had forgotten that we changed Will try and get the extensions added as part of PromiseKit V7 soon! |
I have a proposal for adding cancellation to PromiseKit. PromiseKit is excellent for making code super clean! But I feel like it is missing a good solution for the real-world problem of cancellation. For example, with the current solution the code becomes rather involved for cancelling a chain of promises.
I’ve been working part-time for a couple months (!!) on add-ons for PromiseKit and the PromiseKit extensions that provide very good and complete (I think!) cancellation support. I hope it can be a useful addition to the PromiseKit implementation, and very useful for developers who want a comprehesive way to cancel promises!
I read up on some relevant threads and realize there are drawbacks to having a built-in ‘cancel’ method, so I'm using a delegation approach to leave the existing PromiseKit code unchanged. The core code is a set of wrappers called CancellablePromise, CancellableGuarantee, CancellableThenable, and CancellableCatchable, which delegate to their PromiseKit counterparts. There are also cancellable varients for all the 'after', 'firstly', 'hang', 'race' and 'when' PromiseKit functions. The new methods and functions mirror the public PromiseKit API, with the addition of cancellation support.
The delegate approach has the advantage that promises can be explicitly created with or without cancellation abilities. So the idea is you can get all the advantages without any of the disadvantages.
There is a class called 'CancelContext' that tracks the cancellable task(s) and 'reject' method for each promise in the chain. This allows for easy cancellation of the entire chain including branches. As each promise is initialized its cancellable task and 'reject' method is added to the CancelContext. And as each promise resolves its cancellable task and 'reject' method are removed from the CancelContext. Because these operations can happen on various threads I made the CancelContext code fully thread safe.
I’ve gone through all the PromiseKit extensions and found parts of 9 of them that are cancellable (see below), and implemented cancellable extensions for all of them.
I have a bunch of test cases for both the core code and for the extensions, so I think it is pretty solid.
Packaging
I've gotten this working very well both integrated with CorePromise and as a separate extension. I see 3 options for packaging this:
I will submit pull requests for options 1 and 2. Option 3 looks very similar to the set of projects on my github account: https://github.com/dougzilla32
I personally like option 1 best, then 2, but not so much option 3.
Next steps
I heard back from Max Howell that this might be a good addition to PMK 7. I'll keep the two pull requests for options 1 and 2 up to date with the current PromiseKit code -- the initial pull requests will be for PromiseKit 6.3.4.
I can volunteer to keep the cancellation code up to date as PromiseKit evolves. The other option would be to require contributors to update the cancellation code if any part of the public PromiseKit API is changed.
The text was updated successfully, but these errors were encountered: