Make delay cancelable #26

julasamer · 2018-11-20T10:53:13Z

The delay method is currently not cancelable. I.e., if a requestCancel happens during the delay, the promise resolves successfully anyway:

var promise: Promise<(), NoError>?
func test() {
    promise = Promise(fulfilled: ()).delay(1).always { print("result: \($0)") }
    DispatchQueue.main.asyncAfter(deadline: .now() + 0.5) { self.promise?.requestCancel() }
}

Expected outcome: Promise is resolved after 0.5s with a cancelled result. Actual result: Promise is fulfilled with () after the delay, cancel is ignored.

I implemented another version in an extension that seems to do the job. The fact that the timer isn't actually cancelled is a little ugly though.

public func cancelableDelay(on context: PromiseContext? = nil, _ duration: TimeInterval) -> Promise {
    return self.then {
        result in
        Promise<Value, Error>(on: context ?? .default, {
            resolver in
            var cancelRequested = false
            resolver.onRequestCancel(on: .immediate) {
                _ in
                cancelRequested = true
                resolver.cancel()
            }
            
            _ = Promise<(), NoError>(fulfilled: ()).delay(duration).then(on: context ?? .default) {
                _ in
                guard !cancelRequested else { return }
                resolver.fulfill(with: result)
            }
        })
    }
}

The text was updated successfully, but these errors were encountered:

julasamer · 2018-11-20T10:54:19Z

PS: I know that the docs say that delay is for debugging purposes mainly, but I found it to be a very convenient API for cancelable timers, which are useful in general.

lilyball · 2018-11-20T19:05:43Z

Hi @julasamer, thanks for the issue!

I'd have to double-check the source but I'm pretty sure the current implementation just pipes requestCancel() to the source promise and otherwise does nothing with it. This is consistent with the way cancellation works in general, which is that requesting cancellation shouldn't throw away a usable result if it gets one, it should instead just request that the result never be produced. The reason for this behavior is there are circumstances where you'd want to process a successful result if one is produced, even if you requested cancellation, such as a network request to fetch an image where you attach an observer that caches the image; even if you request cancellation, if the upstream network request is too far along to actually cancel, then it should still cache the image, and you can use a PromiseInvalidationToken on your final observer to avoid handling the result if you don't want it anymore.

That said, this approach was primarily designed around the idea that any delay inherent in a promise is representative of actual work being done. But the .delay() utility is a bit special, in that it is not in fact doing any work, it's just delaying the results. I admit I hadn't really considered the idea of using something like Promise(fulfilled: ()).delay(1) as a timer.

After an admittedly brief amount of thought, here's 3 approaches I'm considering:

Cancel the timer as you suggested. Hopefully any utility observers (like the image cache example above) would be attached before the delay, and therefore would still be able to run their side-effects when the upstream promise is fulfilled. Given that I had primarily intended .delay() to be a debug utility also means the exact semantics of cancellation aren't as important.
Add a separate Promise(fulfilled:after:) (and Promise(rejected:after:)) constructor that runs its own timer, and supports cancellation. This would be a more "official" way of supporting promises-as-timers. But it does seem kind of redundant given how easy it is to write Promise(fulfilled: ()).delay(1).
Change delay to let you control whether requesting cancellation will cancel the timer. This is probably unnecessary complexity.

After outlining all this, I'm inclined to go ahead and do (1).

lilyball · 2018-11-22T05:18:24Z

I looked into the actual implementation. The current implementation does indeed just pipe the cancellation upwards. But more importantly, if the upstream promise does cancel, it still delays handling the cancelled result. By that I mean if you have something like

Promise(result: .cancelled).delay(1)

the delayed promise still waits 1 second before resolving to cancelled itself.

I think preserving this behavior for upstream cancels is important for maintaining delay as a debug utility, but perhaps invoking requestCancel() on the delayed promise itself can then remove that delay for handling cancels.

That said, the specific use-case you've described, of Promise(fulfilled: ()).delay(1), would not actually cancel if you called .requestCancel(), because the upstream promise didn't cancel. There are no promise adapters in this library that will cancel themselves without the upstream promise(s) cancelling and I don't think we should change that for delay.

For your particular use-case I think a separate Promise(fulfilled:after:) constructor is the way to go. So I'll treat this particular ticket as making delay() propagate cancels immediately once .requestCancel() has been invoked on the delayed promise directly (but not cancel if the upstream promise doesn't cancel), and I'll file a separate ticket for Promise(fulfilled:after:).

lilyball · 2018-11-22T05:20:16Z

Filed as #27.

julasamer · 2018-11-23T17:14:33Z

Sounds reasonable. I guess, if I need a cancelable delay somewhere in my promise chain, I can do somePromise.then { Promise(fulfilled: $0, delay: someDuration) }.then {.... It's a little longer than just a delay(someDuration), but it's a rare use case anyway.

Thanks for being so responsive and looking into the issue!

lilyball · 2018-11-24T01:41:58Z

If you need to insert a cancellable delay somewhere in your promise chain, unfortunately what you just pasted won't actually work in the event that the upstream promise doesn't cancel. Even if the upstream promise does respond to cancels, there would still be a race where the upstream promise resolves, then you request cancel prior to the then handler actually running (since the cancel was requested prior to the then handler returning a new Promise, that new promise won't receive the cancellation request, and since the upstream promise already resolved it won't do anything with the cancellation request).

However, you can write your own wrapper that just immediately cancels the returned promise upon request but otherwise delegates to delay. Promise(fulfilled:after:) will still be better if you can use it, as it cleans up its timer immediately upon cancellation (and also throws away the value immediately upon cancellation), whereas this new wrapper delegates to delay's behavior and therefore can't clean any of that up immediately, but as long as you don't care about the result's lifetime being potentially prolonged by the delay (which tbh is a fairly esoteric thing to care about as long as the result itself isn't using significant resources), then this should work:

extension Promise {
    func cancellableDelay(on context: PromiseContext = .auto, _ delay: TimeInterval) -> Promise {
        return Promise(on: .immediate, { (resolver) in
            let delayed = self.delay(on: context, delay).inspect(on:. immediate, { (result) in
                resolver.resolve(with: result)
            })
            resolver.onCancelRequested(on: .immediate) { [upstream=delayed.cancellable] (resolver) in
                resolver.cancel()
                upstream.requestCancel()
            }
        })
    }
}

(note: I didn't actually test this, even for compilation)

More generally, there might be a potential use for a func cancellingImmediatelyUponRequest() -> Promise that returns a new promise that cancels immediately upon request even if the upstream promise doesn't. With that, the above would then turn into return self.delay(on: context, delay).cancellingImmediatelyUponRequest(). I'm not sure yet if this utility function is useful enough to justify its existence though.

lilyball · 2018-11-24T01:47:20Z

I've filed #28 about cancellingImmediatelyUponRequest().

lilyball · 2018-11-27T07:45:49Z

This has been released in v0.4, along with Promise(fulfilled:after:).

lilyball self-assigned this Nov 20, 2018

lilyball added enhancement New feature or request TODO labels Nov 20, 2018

lilyball added this to the Next milestone Nov 20, 2018

lilyball removed the TODO label Nov 20, 2018

lilyball added bug Something isn't working and removed enhancement New feature or request labels Nov 22, 2018

lilyball closed this as completed in fc6de00 Nov 27, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make delay cancelable #26

Make delay cancelable #26

Make delay cancelable #26

Make delay cancelable #26

Comments