Reject cbs which return a value should call resolved handlers
Closed this issue · 11 comments
Hiya
It appears that currently Promises are not compliant with http://promises-aplus.github.io/promises-spec/#point-49
Specifically:
promise2 = promise1.then(onFulfilled, onRejected);
2.2.7.1 If either onFulfilled or onRejected returns a value x, run the Promise Resolution Procedure [[Resolve]](promise2, x).
In other words, the reject handler should be able to handle the error (or rethrow it), so this code:
use Promises qw(deferred);
my $d = deferred;
$d->promise
->then( sub { say "resolve" }, sub { say "reject" })
->then( sub { say "resolve" }, sub { say "reject" });
$d->reject('foo');
should print out:
reject
resolve
when actually it prints out:
reject
reject
The author of AnyEvent::Promises points out this same issue: https://metacpan.org/pod/AnyEvent::Promises#SEE-ALSO
When working with Promises as they are, I found the inability to handle errors and continue down the resolve chain to be an issue, but I assumed that was just how promises worked.
I think this should be changed, but of course it breaks bwc in quite a big way. What do you think? How to handle bwc?
So I discussed this with @wayan (author of AnyEvent::Promises) pretty extensively in #3 and he ultimately decided to write AnyEvent::Promises. I am open to allowing some kind of flag to alter this behavior (see the discussion in the pull-request), but I don't want to alienate existing users with this large of a back-compat break.
Now, that said, my usage of this module is still relatively simplistic, and I would guess that is the case for many of the other users. Your usage is much more extensive and has already shown a number of edge cases and issues which you have done an awesome job of fixing. I suspect that @wayan's usage is also not so simplistic, which is why both of you have felt the need for this feature. And of course, the Promises A+ people I am sure have much more extensive use cases as well. So given all this, there is a voice in the back of my head right now that is telling me that I am wrong and y'all are right.
My original implementation was based upon the Promises-A spec, specifically the YUI and jQuery implementations of it, but what we are talking about here is supporting the Promises-A+ spec. This here might be the key to how to handle this, so allow me to propose something (also, hopefully @wayan will chime in as well).
First, we introduce a means by which the spec can be specified, something like:
use Promises 'A+';
use Promises 'A';
use Promises; # A is the current default
and we leave A to be the current default setting. Docs need to be added, maybe a blog post needs to be written and loud obnoxious notes in the Changelog all need to indicate that A is currently supported, but by $arbitrary_version_number we will switch over to A+ as the default.
Keep in mind, I am only just now sitting down and having my coffee, so there might be big holes in this idea, but please let me know what you think, but be gentle (at least until the caffeine kicks in).
:)
As you know, I'm still new to Promises and am a bit lost as to which specs or test suites should be believed. That said, I think that this behaviour (ie a reject handler returning a value results in the next promise being resolved) actually dates from Promises A, not from A+.
My first witness, m'lord, is the Common JS Promises A test suite: https://github.com/domenic/promise-tests/blob/master/lib/tests/promises-a.js#L158 which is:
describe("[Promises/A] Chaining off of a rejected promise", function () {
describe("when the first rejection callback returns a new value", function () {
it("should call the second fulfillment callback with that new value", function (done) {
rejected(other).then(null, function () {
return sentinel;
}).then(function (value) {
assert.strictEqual(value, sentinel);
done();
});
});
});
Going back to the original Promises A spec, this sentence seems to agree:
This function should return a new promise that is fulfilled when the given
fulfilledHandler or errorHandler callback is finished. This allows promise
operations to be chained together. The value returned from the callback
handler is the fulfillment value for the returned promise. If the callback
throws an error, the returned promise will be moved to failed state.
I don't know about the historical YUI and jQuery implementations (except that jQuery's version was much maligned), but at least YUI has moved to the A+ spec.
Having read through #3 I agree with @wayan about most (all?) his points there, especially as handlers ARE now run in eval blocks. I think the way Promises.pm handles rejections currently is wrong and should be changed...
Re A vs A+, I think we are now (excluding the faulty rejection handling) A+ compatible, at least when running with one of the event loop specific Deferred backends. Am I missing anything? https://github.com/promises-aplus/promises-spec/blob/master/differences-from-promises-a.md
So that brings us back to bwc. First, saying that the current version conforms to 'A' feels wrong... it doesn't :) Second, we don't want these two versions to be compatible. It's quite feasible that two modules from CPAN use different versions of Promises which, when used together, would break in hard to debug ways. (Fortunately, I couldn't find any CPAN modules which depend on Promises yet).
So I'd be tempted to be a bit harsher here, either:
- bump the major version, break bwc, blog etc, or
- release it under a new name, eg: Promises::A
Do you have any idea how many people this would affect? Difficult to know I suppose
Crap, I knew I should have drank more coffee before answering!
I am tempted to just "bump the major version, break bwc, blog etc" on this one.
I know deep down in my heart that the latest spec is the right way to go, I just haven't had enough reading time to study the fine details or programming time with Promises, to prove it beyond a doubt to myself (hence I am living vicariously through you). I also would love to see @wayan's work on AnyEvent::Promises to be folded back into Promises so that we have less splintering of ideas going on.
Do you have any idea how many people this would affect? Difficult to know I suppose
Probably only a few, but the few I do know were using it in production. I suppose if I release it loud enough we can get some good real-world testing out of this too.
Let me sit on it for a little bit and decide, but I am strongly leaning toward breaking back-compat in favor of correctness.
Yeah, the solution seems simple and the change not terribly huge code-wise, just a big(ish) change behavior-wise.
If you want to do some optimistic concurrency here you could work on the patch with the knowledge I am almost certainly accept it.
Also, I pinged @wayan to see if he could come join in on this conversation as well.
Great, I'm on it. Also, re his change of removing RESOLVING, REJECTING? Complies with the spec, so I'd be for it.
That one I was a never really sure would be that useful to remove. I think given the chained nature of promises it might be good to know that you are in the process of resolving and/or rejecting.
But again, I've not done enough promise programming to have encountered a use case, it just seemed a good idea to me at the time.
Really Promises should be used with one of the Deferred backends, in which case none of the callbacks will ever see the intermediate state. The code as it currently stands allows Deferred's to be resolved or rejected more than once, which is also incorrect and which would be fixed while removing the 'ing states.
I'll go ahead and submit my patch and then we can discuss
Hmm, excellent point.
Hello,
I join the conversation with what may be an off topic. From my recent usage of AnyEvent::Promises I found to be quickly dependent on the discussed features of A+, namely
- the resolve / reject semantics
- deferred ("next tick") running of callbacks
- semantics for handler returning thenable (if the handler returns any object with then method I consider it promise and "wait" until then is called)
I also found a "use case" to pass then whatever can be called as code
my $cv = AE::cv;
.....
$p->then($cv)
but it is just a syntax sugar.
So far I never used the state or value methods.
I also found useful to have a function like Promises::collect returning a promise but which value is not an array of arrays (like Promises::collect) but just an array. I am talking about the example from Promises doc
collect(
fetch_it('http://rest.api.example.com/-/product/12345'),
fetch_it('http://rest.api.example.com/-/product/suggestions?for_sku=12345'),
fetch_it('http://rest.api.example.com/-/product/reviews?for_sku=12345'),
)->then(
sub {
my ($product, $suggestions, $reviews) = @_;
$cv->send({
product => $product,
suggestions => $suggestions,
reviews => $reviews,
})
},
sub { $cv->croak( 'ERROR' ) }
);
where the $product, $suggestions, $reviews are not the rest answers but arrays with one element containing the answer.
I try to write more to this topic later.
Hi @wayan
Thanks for weighing in.
semantics for handler returning thenable (if the handler returns any object with then method I consider it promise and "wait" until then is called)
That's certainly part of the spec and we probably should support it. Perhaps somebody wants to mix modules where one use Promises and the other uses eg Future
I also found a "use case" to pass then whatever can be called as code
my $cv = AE::cv;
$p->then($cv)
Yeah, that'd probably be useful. I suppose can check for:
reftype($cb) eq 'CODE' or blessed($cb) and $cb->can('()')
I also found useful to have a function like Promises::collect returning a promise but which value is not an array of arrays (like Promises::collect) but just an array. I am talking about the example from Promises doc
(Also I note that the example in the docs is incorrect)
you mean:
[$product,$suggestions,$reviews]
instead of:
[[$product], [$suggestions], [$reviews]]
That'd be tricky given that each promise can return multiple values... I see in AnyEvent::Promises you just drop any but the first value. That seems counter-intuitive to me.