google / promises

Promises is a modern framework that provides a synchronization construct for Swift and Objective-C.
Apache License 2.0
3.8k stars 293 forks source link

Promise fulfil/reject have broken architecture #74

Closed virl closed 5 years ago

virl commented 5 years ago

Promise class should not have fulfil() and reject() methods — instead, they must be moved to separate class like PromiseSource.

Because currently it is a leak in incapsulation: methods of class that return promises for underlying scheduled operations should not allow fulfillment and rejection of these operations by external user.

shoumikhin commented 5 years ago

Hi Vladimir, thank you for the feedback!

Are you referring the fact we have both canonical concepts of a promise (as an asynchronous provider) and a future (as an asynchronous return object) embodied in one class? Or rather an explicit ability to resolve any promise at anytime from anywhere? Do you suggest to make the pending constructor return an instance of some other class, like PromiseSource or Resolver, and let that one have fulfill and reject methods instead of Promise?

Thanks.

virl commented 5 years ago

@shoumikhin Yes, all of the above.

Promise and its source (aka Future & Promise in terms of some other implementations) should be separate, so that any API (or any protocol or interface for that matter) that returns the Promise object can be sure that user will not be able to interfere with fulfilment and rejection of that promise by API's internal implementation.

Promises should be read-only objects in the sense that they can only be used to monitor pending error/value, not influence their outcome.

Naturally, it implies separating resolving logic into separate class like PromiseSource that will strongly retain corresponding Promise object (but not the other way around).

shoumikhin commented 5 years ago

I think that's an interesting idea. I'll convey it to the rest of folks directly involved into Promises development at Google. Feel free to provide a PR or further details or examples on how you'd like to have that implemented. Thanks again for the suggestion!

virl commented 5 years ago

@shoumikhin I don't think there is much to be added in terms of details.

The rationale behind this proposal is the same as with regular mutable/immutable value and value types in general (as in Swift's value types): when you suppling promise as method argument (or returning it from your method) you want to be sure that method's code (or caller of your method) will not be able to interfere with promise's underlying process.

Such interference should be optional and completely separate from Promise class in form of CancelToken #75 (that can also be used for cancellation of async processes, not just unregistration of promise blocks).

ghost commented 5 years ago

Hey @virl, I think you bring up a good point about the potential pitfalls of exposing an API that returns a promise that the caller can modify in some way (i.e. by calling fulfill/reject). We are currently looking into potential solutions for addressing this and will hopefully have an update shortly.

Thank you!

shoumikhin commented 5 years ago

@virl instead of a separate wrapper class that holds a Promise, what do you think about a PendingPromise subclass with fulfill and reject methods available?

let promise = PendingPromise<Int>()

// elsewhere

promise.fulfill(42)

The plain Promise won't simply have those:

let promise = foo.bar().then { ... }

// elsewhere

promise.fulfill(42)  // compile error

Thus, the intention becomes explicit, and we can still cast PendingPromise to a regular "readonly" Promise and pass it around.

ghost commented 5 years ago

Hi @virl, please let us know what you think about the proposal made by @shoumikhin in the comment above. Feel free to reopen this PR if this approach is worth pursuing.

Thank you!