Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make delay cancelable #26

Closed
julasamer opened this issue Nov 20, 2018 · 8 comments

Comments

Projects
None yet
2 participants
@julasamer
Copy link

commented Nov 20, 2018

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)
            }
        })
    }
}
@julasamer

This comment has been minimized.

Copy link
Author

commented Nov 20, 2018

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

This comment has been minimized.

Copy link
Owner

commented Nov 20, 2018

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:

  1. 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.
  2. 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).
  3. 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 lilyball self-assigned this Nov 20, 2018

@lilyball lilyball added this to the Next milestone Nov 20, 2018

@lilyball lilyball removed the TODO label Nov 20, 2018

@lilyball

This comment has been minimized.

Copy link
Owner

commented Nov 22, 2018

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 lilyball added bug and removed enhancement labels Nov 22, 2018

@lilyball

This comment has been minimized.

Copy link
Owner

commented Nov 22, 2018

Filed as #27.

@julasamer

This comment has been minimized.

Copy link
Author

commented Nov 23, 2018

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

This comment has been minimized.

Copy link
Owner

commented Nov 24, 2018

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

This comment has been minimized.

Copy link
Owner

commented Nov 24, 2018

I've filed #28 about cancellingImmediatelyUponRequest().

@lilyball lilyball closed this in fc6de00 Nov 27, 2018

@lilyball

This comment has been minimized.

Copy link
Owner

commented Nov 27, 2018

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.