Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix #46 #63

Merged
merged 2 commits into from
May 20, 2017
Merged

Fix #46 #63

merged 2 commits into from
May 20, 2017

Conversation

bpolaszek
Copy link
Contributor

The all() function, which resolves a stack of promises, has now an option to resolve new promises that were added to the stack during the resolution of the 1st ones.

use GuzzleHttp\Promise\Promise;
use GuzzleHttp\Promise\PromiseInterface;
use function \GuzzleHttp\Promise\all;

$promises = new ArrayIterator();
$promises['Bill'] = new Promise(function (PromiseInterface $promise) use ($promises) {
    $promise->resolve('Clinton');
    $promises['Donald'] = new Promise(function (PromiseInterface $promise) {
        $promise->resolve('Trump');
    });
});

all($promises, true)->then(function ($result) {
    var_dump($result);
})->wait();

outputs:

array (
  "Bill" => "Clinton"
  "Donald" => "Trump"
)

Without this only Bill Clinton would have been resolved.
To avoid BC breaks this option is set to false by default.

Use case:
Call an API which have paginated results: for each page, add a new promise to the stack.

$promise = $promise->then(function ($results) use ($recursive, &$promises) {
foreach ($promises AS $promise) {
if (\GuzzleHttp\Promise\PromiseInterface::PENDING === $promise->getState()) {
return all($promises, $recursive);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feeds promises into an EachPromise, even if they're fulfilled, right? Won't that invoke the fulfilled / rejected callbacks of the already settled promises potentially multiple times?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good point.

I oriented my tests on wait functions that return a fixed value, and assumed the already settled promises won't have their wait function called anymore, since they should be already resolved or rejected. I have to check this in a deeper way. Do not merge for the moment.

If I was wrong, what do you suggest to loop over the new pending promises? At first I thought putting them in a new ArrayIterator but performance impact apart, this would force the original $promises type hint and if the user defined a MyPromisesStackIterator with an addNewPromise() method, for instance, he would not be able to call that new ArrayIterator::addNewPromise().

That's why I was attached in keeping the original iterable passed to the function.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello again,

I just checked this out, and all is fine: the wait function is not called again on a non-pending promise, so we can safely loop over the original iterator again without worrying about promises that may be settled multiple times.

I added that point into the unit tests to ensure that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for looking at this! Sorry, I should have clarified -- I meant these callbacks: https://github.com/guzzle/promises/blob/master/src/EachPromise.php#L33.

Copy link
Contributor Author

@bpolaszek bpolaszek Mar 31, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi there,

I checked again when using all(), new EachPromise() and combining them, and the onFulfill callbacks aren't called twice.
Try this:

require_once __DIR__ . '/vendor/autoload.php';

use GuzzleHttp\Promise\EachPromise;
use GuzzleHttp\Promise\FulfilledPromise;
use GuzzleHttp\Promise\Promise;
use GuzzleHttp\Promise\PromiseInterface;
use function GuzzleHttp\Promise\all;
use GuzzleHttp\Promise\RejectedPromise;

// Create food promises using "new EachPromise()"
$config         = [
    'fulfilled' => function ($value) {
        printf('I love %s' . PHP_EOL, $value);
    },
    'rejected'  => function ($value) {
        printf('I hate %s' . PHP_EOL, $value);
    },
];
$foodPromises   = new ArrayIterator();
$foodPromises[] = new FulfilledPromise('vegetables');
$foodPromises[] = new Promise(function (PromiseInterface $promise) use (&$foodPromises) {

    $promise->resolve('hamburgers');

    // Add another promise into the stack during resolution
    $foodPromises[] = new Promise(function (PromiseInterface $promise) {
        $promise->resolve('burritos - but no one cares'); // should not be called
    });

});
$foodPromises[] = new RejectedPromise('cabbage');
$foodPromise    = (new EachPromise($foodPromises, $config))->promise();

// Now create drink promises using "all()" function
$drinkPromises   = new ArrayIterator();
$drinkPromises[] = new FulfilledPromise('beer');
$drinkPromises[] = new Promise(function (PromiseInterface $promise) use ($drinkPromises) {

    $promise->resolve('whisky');

    // Add another promise into the stack during resolution
    $drinkPromises[] = new Promise(function (PromiseInterface $promise) {
        $promise->resolve('diet coke - but no one cares'); // should be called
    });

});
$drinkPromise    = all($drinkPromises, true) // Using all() function with recursion
    ->then(function ($results) use ($config) {
        array_walk($results, $config['fulfilled']);
    })
    ->otherwise(function ($reason) use ($config) {
        call_user_func($config['rejected'], $reason);
    });

// Combine all promises and wait
all([$foodPromise, $drinkPromise])->wait();

It should output:

I love vegetables
I hate cabbage
I love hamburgers
I love beer
I love whisky
I love diet coke - but no one cares

As you can see there are no duplicates.

@bpolaszek
Copy link
Contributor Author

Hello @mtdowling,

Is everything OK for you?

Thank you,
Ben

@mtdowling mtdowling merged commit 09e549f into guzzle:master May 20, 2017
@mtdowling
Copy link
Member

Sorry for the delay. Looks good to me. Thanks!

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

Successfully merging this pull request may close these issues.

2 participants