makes the exception handling closer to Promises/A+ specification

[wayan]

• even onFulfilled handler can be undef
• exceptions thrown in onFulfilled an onRejected handlers (args of then) are caught
• resolve and reject method prevented to be called more than once
• if onRejected handler returns value (does not throw exception) the promise is fulfilled
• intermediate states (REJECTING, RESOLVING) dropped
• the promise of deferred object is not a property, but is created on the fly ( to prevent circular reference)

[stevan]

First of all, thanks for the patch, please do not let my disagreements with some of these changes discourage you, I am open to further discussion on all points.

That said, I am a little worried this does change too much.

• I am unclear of what value there is in allowing onFulfilled to be undef, can you provide a reasoning beyond "that is what the spec says"?
• I am fine with catching the exceptions in onFulfilled and onRejected, but I would prefer to use Try::Tiny instead of the raw eval/if ($@) construct
• preventing resolve and reject being called more then once is fine, I like that
• I am not sure I understand "if onRejected handler returns value (does not throw exception) the promise is fulfilled", are you saying that if onRejected doesn't throw an exception, the state moves to onFulfilled? If so, I do not agree with this, there are many ways to handle errors in Perl and I do not want to force this convention on people. In particular I suspect this would break the AnyEvent condvar->croak pattern since the exception for that is thrown upon condvar->recv and not exactly inside the handler.
• I am not sure removing the intermediate states is a good idea, it assumes that resolve/reject happen atomically, which is typically safe for asynchronous code, but not in threaded environments, I would prefer to leave those in.
• regarding the circular reference, I think this is better dealt with using Scalar::Util::weaken

[wayan]

• Any pure error handler will have it undef.

For example I have a pipeline of objects (like Message::Passing, they are connected via output_to attribute). Each of the objects but the first one has a method consume which accepts message and returns promise.

There is a producer (STOMP subscription) which - when message appears in a queue - calls asynchronously:

$this->output_to->consume($message)->then(sub {
    # if message was processed, remove it from the queue
    $this->ack($message); 
}, sub {
# what to do ?
});

There is an object (error handler) further on the pipe which lets the message go through and only if the processing fails stores the message for reinspection and returning to the pipeline.

sub consume {
    my ($this, $message) = @_;
    return $this->output_to->consume($message)->then(undef, sub {
        my ($this, $error) = @_;
        $this->store_error_message($message, $error);
        return;
    });
}

By this we make the promise for producer eventually fulfilled and the message is acknowledged.

• ok
• ok
• by this I mean that (according to spec) that having rejected promise $p and

my $q1 = $p->then(sub {});
my $q2 = $p->then(sub {}, sub {});
my $q3 = $p->then(sub {}, sub { die $_[0] });

$q1 and $q3 are rejected with the same reason as $p, but $q2 is fulfilled with empty result []. Which, now I see, implies that you cannot reject a promise without $reason, which is logical.

• Regarding intermediate states I found a bit confusing to have two sets of states (one for specification). I also felt that state is changed for once and only before the "next" promises are resolved and the intermediate state is just a flag "callbacks are running".
• If the promise object is stateless why is it better to have fixed one in $deferred?
• There is also another idea from Q javascript framework (https://github.com/kriskowal/q/wiki/API-Reference#qdefer): to make cleaner distinction between resolvers (deferred) and promises. Resolvers can resolve and reject while promises can 'then' and examine state. It means probably to make $deferred->then private (_then).

[stevan]

Lets organize these so we can keep track better.

Agreed

• okay, your reasoning is fine, I will accept that change
• you already agreed to this
• you already agreed to this

Not Agreed

• I am not sure if you are agreeing with me or not. Can you clarify?
• Yes, I think it is important to note the difference between "not yet rejected or resolved", "rejected or resolve and callbacks are running" and "callbacks have run", especially in something like a threaded environment where an interrupt could occur.
• I agree with the distinction between the producers that use the Promises::Deferred API and the consumers which use the Promises::Promise, but I don't think this needs to be that strict. It is the classic Perl idea that you won't come into my house because I asked you not too, and not because I have a shotgun pointed at the door. However I don't see how not re-using the same Promise instance changes this.

[wayan]

• For onRejected handler semantics I cannot clarify, because I did not understand your comment "are you saying that if onRejected doesn't throw an exception, the state moves to onFulfilled ..." . I say that if onRejected (or onFulfilled) doesn't throw an exception, the state moves to fulfilled (from unfulfilled). I think this is what the spec says. As I read the spec the state of promise is set according to fact whether the handler threw an exception regardless which handler was called. I may miss the point of disagreement here.
• Intermediate states: It may be important to know whether there are some callbacks running, but is it really a state? But I'm OK as long as there are the "the three possible states according to the spec".
• For reusing promise instance I have no strong argument neither for reusing nor creation on the fly. (It doesnot affect the producer/consumer API however - it was a different point).

[stevan]

Not Agreed

• I guess I am still confused by this, can you point me to line in the spec which explains this?

Agreed

• I would prefer to leave the intermediate states in there, I do think it is a state.
• I would prefer to re-use instances, I think it is more efficient and I see no reason behind always returning a new one all the time

[wayan]

• See 2.2.7.1 at http://promises-aplus.github.io/promises-spec/ , it clearly makes no distinction between onFulfilled and onRejected handlers.

[stevan]

Okay, so basically the only change for this is to wrap this line (https://github.com/stevan/promises-perl/blob/master/lib/Promises/Deferred.pm#L102) in a try/catch block to catch any exceptions which might be thrown, and then to call reject with the exception as the argument if one is thrown. Is that correct?

[wayan]

not at all. You also need to change the line 110 from

$d->$method( @results )

to

$d->resolve( @results )

[stevan]

I don't understand that change, it would cause this line to not work correctly.

https://github.com/stevan/promises-perl/blob/master/lib/Promises/Deferred.pm#L86

[stevan]

Oh wait, I see now, I wasn't reading the spec correctly. I think that is just wrong behavior, and it would seriously break backwards compatibility.

[stevan]

I believe that this part of the spec is there because things like throwing exceptions is pretty much the only way people do errors in Javascript, in Perl this is not the case.

[wayan]

I think this beviour is good :-) because it allows you to escape from onRejected handler and thus nice error handlers.

sub handle_error {
    my $p = shift();
    $p->then(undef, sub {
        my ($error) = @_;
         die $error if isFatalForMe($error);
         storeTheError($error);
         return;
    });
}

while with your semantics, you have to change "the chain" and create one more promise.

sub handle_error {
    my $p = shift();
    my $q = deferred();
    $p->then(undef, sub {
        my ($error) = @_;
        if (isFatalForMe($error)){
            $q->reject($error);
        }
        else {
             storeTheError($error);
             $q->resolve();
        }
    });
    return $q->promise;
}

But yes, it would definitely break the backward compatibility of Promises.pm.

[stevan]

I think I need to keep the backwards compatibility, so I will have say no to this feature, but as a compromise I would accept a patch that allowed this behavior to be used based on a flag, something like.

Promises::Deferred->new( resolve_exceptions => 1 ) 

It would also mean having to have Promises::deferred pass through parameters, but that is a simple change. Would that work for you?

[stevan]

@wayan - just FYI, I need to make this change soon, so just be aware that the master branch will have changes in it.

[wayan]

"Would that work for you?" . Probably not. I understand that this is radical twist in behaviour. Better to keep the semantics of Promises.pm simple and unchanged and spare the arguments of defer for future use.

For myself I will probably make some very simple implementation in my private namespace or under different name on CPAN (AnyEvent::Promise?). I think to make it AnyEvent specific and thus complying to 2.2.4 (the handlers running outside of the current executon stack) of the spec also.

[stevan]

@wayan - another option would be to make _wrap part of the public API so that you can easily subclass it and adjust the behavior accordingly. Additionally creating an extension point for your AnyEvent specific 2.2.4 behavior as well. Does that work?

[stevan]

@wayan - are you still interested in re-doing this patch? Or have you decided to write one yourself instead?

[wayan]

@stevan - sorry for delay. Finally I have decided to write my own simple implementation instead with the same interface.

Regarding _wrap it would probably work to have in subclass:

around wrap => sub {
    my $orig = shift;
    my $code = $orig->(@_);
    sub {
        AE::postpone($code);
    };
};

but It would make the meaning of intermediate states less clear.

[stevan]

@wayan - no worries, I will implement all the bits we agreed upon anyway. Thanks for the initial work and the discussion.