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

Loose interpretation of promise/a+ #22

Closed
stefanpenner opened this issue May 12, 2014 · 9 comments
Closed

Loose interpretation of promise/a+ #22

stefanpenner opened this issue May 12, 2014 · 9 comments

Comments

@stefanpenner
Copy link

My Objective-C isn't fantastic, so I might be totally wrong about this.

But I have some promise experience

Anyways, a quick skim leaves me concerned:

Given: a settled promise, https://github.com/mxcl/PromiseKit/blob/master/PromiseKit.m#L137 appears to synchronously invoke the callback of a chained then.

This would be a serious departure from the spec.

The callbacks must not be invoked until the execution context stack is once again fresh.
see: https://github.com/promises-aplus/promises-spec#the-then-method specifically point 4. Without this, we unleash Zalgo onto our code base.

@mxcl
Copy link
Owner

mxcl commented May 12, 2014

Thanks for the review.

You are right, the then is executed synchronously if the promise has been resolved.

Execution contexts seem rather JavaScript specific. Is there any portable reason that synchronously invoking the then is unsafe?

@stefanpenner
Copy link
Author

Basically, if something can every be asynchronous is should always be asynchronous or you will unleash zalgo this is a promise a+ invariant.

Sorry for the JS but a super simple example of the issue.

var a = 1;
User.find(1).then(function() {
  a++;
  a === 3; // should always be 3, in the case of PromiseKit, this would depend on data locality
});
a++;
a === 2; // should always be one, but in case of PromiseKit, it would depend on data locality

Typically the only constraint is, information cannot be extracted from a promise (regardless of data locality) until the next clean frame.

Feel free to ping me on gtalk: stefan.penner@gmail.com or irc: iamstef
Also, feel free to pop by #promises on freenode.

I would love to help you get this implementation aligned with a+ (as it advertises)

@mxcl
Copy link
Owner

mxcl commented May 12, 2014

Yes, I see. I'll amend that. Thanks.

@stefanpenner
Copy link
Author

@mxcl is their interest in conforming to the spec?

@mxcl
Copy link
Owner

mxcl commented May 12, 2014

Yes for sure. As much as it makes sense to from the Objective-C point of view. I know the spec was the result of much work, research and real-world experience so believe it to be a solid goal.

@mxcl
Copy link
Owner

mxcl commented May 18, 2014

Thanks for pointing this stuff out. As I have come to fix I have realized its importance.

@stefanpenner
Copy link
Author

@mxcl 👍 , if you have further questions let me know.

@mxcl mxcl closed this as completed in dc87284 May 19, 2014
@nikolaykasyanov
Copy link

This part of the spec probably makes sense on server-side, but it breaks some scenarios in UI code. For example, your API client class can have the following method:

- (Promise *)loadPhotoForUser:(User *)user

It checks local in-memory cache and returns resolved promise if there's a matching image, otherwise network request is performed and promise resolved/rejected accordingly.

Your client code may look like:

Promise *userPhoto = [[client loadPhotoForUser:me] then:^(UIImage *photo) {
    self.imageView = photo;
}];

The point is, if -then: is always async, image from in-memory cache will hit image view only on the next run loop iteration, and user may notice flickering.

ReactiveCocoa's signals, for example, could be completely synchronous and I haven't noticed any Zalgo, just FYI.

@mxcl
Copy link
Owner

mxcl commented Jun 23, 2014

The solution is: call -value first, and branch.

The situation where the callback is executed out-of-order becomes ever more possible the more promises involved in your system, and then, eventually, one day, you are bitten by this issue, and like a race-condition, it doesn't happen always, just sometimes, and you can’t figure out the cause or the source.

Probably we can provide an immediate then option. As I hate the check and branch too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants