stevan / promises-perl

An implementation of Promises in Perl
31 stars 29 forks source link

Provide event-loop specific plugins #8

Closed clintongormley closed 10 years ago

clintongormley commented 10 years ago

Hiya @stevan

I'm very much liking Promises, but have run into an issue with deep recursion. You do warn about it:

However, the one important difference that should be noted is that "Promise/A+" strongly suggests that the callbacks given to then should be run asynchronously (meaning in the next turn of the event loop). We do not do this because doing so would bind us to a given event loop implementation, which we very much want to avoid.

...but I didn't quite comprehend how important this issue was until too late.

For a lot of use cases, this limitation is not important. The stack size doesn't get that big before a return which clears this all up. However, I have a couple of use cases where the size of the stack becomes a major problem.

This could be resolved by providing event-loop specific implementations, eg for AnyEvent all you'd need is something like:

package Promises::Deferred::AE;

use AE;
our @ISA = 'Promises::Deferred';

sub _notify {
    my ($self, $callbacks, $result) = @_;
    AE::postpone { $_->( @$result ) } foreach @$callbacks;
    $self->{'resolved'} = [];
    $self->{'rejected'} = [];
}

With an AE version and a Mojo version I think you'd cover most Perl async applications(?). Would you be willing to consider something like this?

clintongormley commented 10 years ago

... I realise that this can be done within the code using Promises too, but that puts more burden onto the user, when this is an issue that could be solved centrally.

stevan commented 10 years ago

@clintongormley

Yes, I would be open to this. I actually pondered a similar solution when I first wrote this but decided to punt until it actually became a problem. Along with the subclassing I thought it should be possible to use the generator feature of Sub::Exporter to allow for something like:

use Promises -backend => 'AE', qw[ collect deferred ];

After which any user code would not to need changing (in fact, the whole reason for the overly simplistic helper function deferred was to allow for this in the future).

Make sense?

clintongormley commented 10 years ago

Yes it does. I've had a bash at implementing this for AnyEvent. I'm not that familiar with Sub::Exporter so the syntax that I came up with is a bit more clunky:

use Promises backend => ['AE'], qw( collect deferred);

See pull request.

Am I doing it wrong?

stevan commented 10 years ago

No, that is likely the correct Sub::Exporter syntax, I was pulling my version out of thin air without consulting the docs.

stevan commented 10 years ago

I will review the pull request and almost certainly merge it right in. Is this something you need ASAP? meaning, do you want me to release it now? or do you want to actually use it for a bit first and make sure it feels right?

clintongormley commented 10 years ago

No I don't need it right this minute. Also I need to add tests and docs in the PR before you merge it. Just wanted to check that we were thinking the same way.

I'm working on async backend for Elasticsearch.pm at the moment, currently just AE, but I want to add Mojo too. So i should include a Mojo backend for Promises as well.

thanks for looking at the PR - let me know if it is OK, and I'll ping you when I have done the rest

clintongormley commented 10 years ago

I'm starting to think that the interface for event-loop specific Promises is wrong. I'm having to choose the backend I want to use for Promises wherever I use them, when really this is something that should be specified by the end user. In other words, in my modules, I want to write:

package MyModule;
use Promises qw(deferred);

And have the end user write:

use Promises backend => 'AE';
use MyModule;

An application typically only has one event loop running (or in the rare case where there are more, only one MAIN event loop). What would you think about making the backend to use a class variable which gets set once by the user?

stevan commented 10 years ago

Yes, this is much nicer, very nice catch!

clintongormley commented 10 years ago

Hiya Stevan

I'm getting close to releasing my code. Any chance you could release a new version of Promises?

many thanks

stevan commented 10 years ago

Sure, i will cut a release tomorrow morning.

stevan commented 10 years ago

Sorry, my monday got away from me and I am in meetings all day today (tuesday), but I will get this out the door on wednesday.

clintongormley commented 10 years ago

no worries - thanks

stevan commented 10 years ago

@clintongormley code has been sent up to PAUSE, should be on CPAN shortly, sorry for the delay.

clintongormley commented 10 years ago

No worries, and many thanks!