the closure for ensure() should return a Promise to make the containing chain composable

[ilyathewhite]

PromiseKit 6 (but applies to 4 as well)

Even though the ensure operator returns a promise, it assumes that its closure is synchronous; calls it and then immediately returns the promise that it's being applied to. The assumption is that ensure is expected to run at the end of the chain, so it doesn't need to be asynchronous. This works if ensure is used only in that way but breaks if we want to turn the chain into a reusable (compound) promise.

I wrote more about it here. It's possible to implement an overload of ensure this way using recover. I think that it would be nice to have this overload as part of PromiseKit.

extension Promise {
    /// Boolean state that is used for sharing data between
    /// promises. It needs to be a class because a struct would
    /// be just copied.
    private class BoolBox {
        /// The stored value
        var value: Bool

        init(_ value: Bool) {
            self.value = value
        }
    }

    /**
     The provided closure executes when this promise resolves.

     This variant of `ensure` executes just as any other `then`
     clause which allows the provided closure to have
     ascynchronous code. Unlike `then`, the closure executes on
     either fullill or reject (same as `ensure`). If the closure
     is rejected, this rejects the containing chain as well.

     - Parameter on: The queue to which the provided closure dispatches.
     - Parameter execute: The closure that executes when this promise fulfills.
     - Returns: A new promise that resolves when all promises
     returned from the provided closure resolve.
     */
    public func ensureAsync(on q: DispatchQueue? = conf.Q.return, execute body: @escaping () -> Promise<Void>) -> Promise {
        let executedBody = BoolBox(false)

        return self.then(on: q) { value -> Promise in
            executedBody.value = true
            return body().then(on: q) { () -> Promise in
                return .value(value)
            }
        }
        .recover(on: q) { (error) -> Promise in
            if !executedBody.value {
                return body().then(on: q) { Promise(error: error)}
            }
            else {
                return Promise(error: error)
            }
        }
    }
}

[mxcl]

Yes I've needed something like this before, seems like this would be enough:

func ensure(on: DispatchQueue? = conf.Q.return, _ body: () -> Promise<Void>) -> Promise {
    let promise = Promise<T>(.pending)
    pipe { result in
        on.async {
            body().pipe {
                promise.box.seal(result)
            }
        }
    }
    return promise
}

I think this would not cause ambiguity, so we can call it ensure still. But we'd have to test for that. Ambiguity when using Swift is annoying to work around in such cases.

[ilyathewhite]

I am not sure if this implementation as it is now propagates the errors. If body() rejects, the error will not be returned to the caller of ensure. We need to combine the two results. But using the internal APIs is definitely faster.

[mxcl]

True. Which makes me think that we can only have a Guarantee<Void> version of this, and probably this is why I didn't implement this before.

Because if the promise you return rejects which error and the promise we are part of is already rejected which Error do we propagate?

```swift
func ensure(on: DispatchQueue? = conf.Q.return, _ body: () -> Guarantee<Void>) -> Promise {
    let promise = Promise<T>(.pending)
    pipe { result in
        on.async {
            body().done {
                promise.box.seal(result)
            }
        }
    }
    return promise
}

[mxcl]

It seems reasonable that any async thing you do in ensure should not be able to error. What do you think?

[ilyathewhite]

I think that typically it wouldn't, and most closures could return a Guarantee. But in some rare cases there could be code that might error, and in those cases, it would be nice to get that error instead of ignoring it. The chain may completely roll back something but still recover rather than being left in an inconsistent state.
A requirement to always return a Guarantee may be enough in most cases, but in the few cases where it won't work, the user will be stuck.

[ilyathewhite]

In terms of which error to propagate: ideally, there could be an inner / nested error; but short of that, I think it should be the first error, the one before calling the closure. The other one is secondary; if it can't be returned as a nested error, logging it could be the next best thing. It's definitely an edge case, but this would give the user more tools to deal with errors.

[mxcl]

We once nested errors for PMKWhen so you could know the index of the promise that error'd and it led to bad error handling since you may handle the error very far from the error site and then you have to check for the error you want and the nested error type and if that nested error is the error you want.

So I think it better we don’t do that.

Do you have examples where you have to return a Promise?

[ilyathewhite]

The nested error would be more for debugging than handling the error, but logging it and returning the original error would also work.

Examples:

Any cleanup action that may be asynchronous. Animation is the first generic example that comes to mind. More specifically: animated dismissal of a view controller / hiding the UI that stared the chain.

Another example could be showing additional UI at the end fo the chain (no matter successful or not) before completing the chain.

[mxcl]

Both these examples would be Guarantees though, right? Any examples which would return Promise? I looked back over every time I ever needed such a thing and had no examples that could error.

In the past I had a ticket with some networking cleanup though. That ofc. could error, but certainly it's a rarer situation IME.

[ilyathewhite]

I thought more about this. The semantics does become crystal clear if it's a Guarantee while making it a Promise introduces ambiguity, so maybe a Guarantee is a better choice. I also cannot think of any real use cases for Promise and was more thinking about making it as generic as possible.

[mxcl]

K cool. I've prepared a PR.

[ilyathewhite]

Great, thank you!

[mxcl]

Merged, will release when I have stable master.