Don't throw an error when resolve() is called twice

[felixfbecker]

That is also how Promises/A+ spec behaves. It is very useful to register the fulfill function as an event handler for example and get the promise resolved as soon as it is called, no matter if it will be called more times afterwards.

[evert]

I definitely didn't get this form Promises/A+ and it also seems weird to be able to do this. In my mind a promise effectively should become immutable as soon as it has been resolved (or failed).

Maybe I'm not understanding this correctly. If so, please let me know. I tried fulfilling a promise twice in firefox, and then callbacks only get triggered the first time, and only the first value gets retained. I was surprised to see no errors though, but it definitely feels wrong.

[felixfbecker]

Of course the handler should only be called once. But subsequent calls should just be ignored.
Here is the use case:

$promise = new Promise;
$writer->onMessage([$promise, 'resolve']);
$promise->then(...);

Should resolve with the first message. Instead, it will throw an error on the second, so I have to do this boilerplate code everywhere:

$promise = new Promise;
$writer->onMessage(function ($msg) use ($promise) {
     if ($promise->state === Promise::PENDING) {
          $promise->fulfill($msg);
     }
});

[felixfbecker]

This behavior is speced here: https://promisesaplus.com/#point-59

2.3.3.3.3 If both resolvePromise and rejectPromise are called, or multiple calls to the same argument are made, the first call takes precedence, and any further calls are ignored.

[evert]

Hi Felix,

That's interesting. However, that paragraph does not refer to being able to resolve a promise more than once using the executor function.

What that refers to is the value that is returned from one of the two then callbacks:

$promise->then(
    function() {

        return $thisvalue;

    }, function() {

       return $orThisOne;

   }
);

If in the previous example $thisValue or $orThisOne is an object with a then function, in that case it should internally call then on that object, and chain the result of that function to whatever the first then returned. This is basically happening inside the promise in the case where there were no rejections (heavily simplified):

$subPromise = new Promise();
// $this is the main promise.
$result = $this->onResolved($value);
if ($result instanceof Promise) {
   $result->then(function($value) { 
       $subPromise->resolve($value);
   });
}

This line in the spec:

2.3.3.3.3 If both resolvePromise and rejectPromise are called, or multiple calls to the same argument are made, the first call takes precedence, and any further calls are ignored.

Only refers to a situation where in the past example $result would call the callbacks passed to then more than once. This basically ensures that the Promise/A+ specification can interop with other implementations of Promise-like/defer objects that have a then method, but are themselves not conformant.

So it doesn't specifically say that a Promise can be resolved more than once, and subsequent ones ignored, it just says that if the result of a then callback returns a promise-like object, then it should try to accommodate situations where that promise-like object calls the callbacks set on then more than once.

This is in direct conflict with the other points in the spec:

https://promisesaplus.com/#point-29

it must not be called more than once.

And this is more or less proof that "point 59" is solely for interopability with promises that are not Promise/A+ compliant.

So with that being out of the way, the question then is does this change make sense for sabre/event? I would say that the benefit of being alerted when you attempt to resolve a promise more than once is a good one, and it has prevented me from introducing bugs or creating problems that would otherwise be silently ignored.

I think this is a good thing considering that Promises are already quite hard to grasp. It's one extra protection that prevents you from making mistakes, and I would say that that's a good thing.

I don't know if you care about using the Emitter in your example, but if you are, you could rewrite your sample to something like this:

$promise = new Promise;
$writer->once('message', [$promise, 'resolve']);
$promise->then(...);

[felixfbecker]

Might have read the spec wrong there, but it clearly is what all JavaScript engines implemented. ECMAScript spec also does not state that an error should be thrown:

25.4.1.4
FulfillPromise ( promise, value)
When the FulfillPromise abstract operation is called with arguments promise and value the following steps are taken:
Assert: the value of promise's [[PromiseState]] internal slot is "pending".
Let reactions be the value of promise's [[PromiseFulfillReactions]] internal slot.
Set the value of promise's [[PromiseResult]] internal slot to value.
Set the value of promise's [[PromiseFulfillReactions]] internal slot to undefined.
Set the value of promise's [[PromiseRejectReactions]] internal slot to undefined.
Set the value of promise's [[PromiseState]] internal slot to "fulfilled".
Return TriggerPromiseReactions(reactions, value).

Assert here just means:

A step that begins with “Assert:” asserts an invariant condition of its algorithm. Such assertions are used to make explicit algorithmic invariants that would otherwise be implicit. Such assertions add no additional semantic requirements and hence need not be checked by an implementation. They are used simply to clarify algorithms.

This also makes a lot of sense, because neither Promises/A+ nor ES6 define any way to inspect the promise state. Something like $promise->state === Promise::PENDING is not possible.

Imo the error is better suited for a log message, like you get with bluebird for example, which also warns you against not-handled rejected promises. You could register a PSR-compatible logger interface.
You are right, using the Emitter would work here, but I am avoiding it because I need to type against an interface like this:

interface ProtocolReader {
  onMessage(callable $listener);
}

and with the event name as a string that obviously doesn't work. I could introduce a onceMessage() of course, but I actually found myself having to write that boilerplate in other places too. I am probably just used to how it works in JS and rely on that behavior there a lot so it was a confusion point for me, but I don't feel to strongly about it. But imo following the spec would be better.

[evert]

I think I maintain that the little bit of boilerplate is a bit annoying, but I think the feature is useful enough.

Here's a quick function can use to resolve a promise, but only if it hasn't already:

function resolveOnce($promise, $value) {
  if ($promise->state === Promise::PENDING) {
     $promise->resolve($value);
  }
}

That should alleviate your biggest frustration. I also wanted to comment further on this:

This also makes a lot of sense, because neither Promises/A+ nor ES6 define any way to inspect the promise state. Something like $promise->state === Promise::PENDING is not possible.

This is true, but the ES6 promise doesn't have a resolve function either. You're required to pass an executor function to the constructor and call the local resolve function there. Granted, it allows you to call resolve twice error-free, but my point is that you don't really need to know the state, because it's trivial to keep track if it was resolved or not. I added a few features that (I feel) work better with PHP's synchronous nature. ->done() is another one that you won't find in ES6 for good reasons.

Anyway, closing this. Hope the function alleviates most of the pain for you!