Promise\settle() is not working properly

[mxr576]

Q	A
Bug?	yes
New Feature?	no
Version	guzzlehttp/guzzle 6.3.3 guzzlehttp/promises v1.3.1 guzzlehttp/psr7 1.4.2 php-http/guzzle6-adapter v1.1.1

Actual Behavior

If the first HTTP call (promise) returns HTTP >= 400 (which triggers and exception in Guzzle by default) then the adapter throws the exception for the first HTTP call instead it would return the result of all promises.

<?php

require_once "vendor/autoload.php";

use GuzzleHttp\Client;
use GuzzleHttp\Promise;
use GuzzleHttp\Psr7\Request;

$client = new Client(['base_uri' => 'http://httpbin.org/']);

$pluginClient = new \Http\Client\Common\PluginClient(
  new Http\Adapter\Guzzle6\Client($client), []
);

// Initiate each request but do not block
$promises = [
  'image' => $pluginClient->sendAsyncRequest(new Request('GET', 'status/404')),
  'png'   => $pluginClient->sendAsyncRequest(new Request('GET', 'status/200')),
  'jpeg'  => $pluginClient->sendAsyncRequest(new Request('GET', 'status/200')),
  'webp'  => $pluginClient->sendAsyncRequest(new Request('GET', 'status/404'))
];

try {
  // Wait for the requests to complete, even if some of them fail
  $results = Promise\settle($promises)->wait();
}
catch (\Exception $e) {
  echo $e->getMessage();
}
exit(0);

Expected Behaviour

settle() should "Wait for the requests to complete, even if some of them fail". So even if the first HTTP call returns HTTP >= 400 an exception should not be thrown.

http://docs.guzzlephp.org/en/stable/quickstart.html#concurrent-requests

<?php

require_once "vendor/autoload.php";

use GuzzleHttp\Client;
use GuzzleHttp\Promise;

$client = new Client(['base_uri' => 'http://httpbin.org/']);

// Initiate each request but do not block
$promises = [
  'image' => $client->getAsync('status/404'),
  'png'   => $client->getAsync('status/200'),
  'jpeg'  => $client->getAsync('status/200'),
  'webp'  => $client->getAsync('status/404')
];

try {
  // Wait for the requests to complete, even if some of them fail
  $results = Promise\settle($promises)->wait();
}
catch (\Exception $e) {
  echo $e->getMessage();
}
exit(0);

Steps to Reproduce

See the code snippets above.

Possible Solutions

For the first sight it seems the adapter should also wrap the result of all promises to a Promise object and return that instead of returning the results directly. At least this is what Guzzle Promise's working code indicates for me. https://github.com/guzzle/promises/blob/v1.3.1/src/Promise.php#L69

[mxr576]

So even client plugins can not throw an exception when settle() is in use with Guzzle6 adapter because you get the same unexpected behaviour.

<?php

require_once "vendor/autoload.php";

use GuzzleHttp\Client;
use GuzzleHttp\Promise;
use GuzzleHttp\Psr7\Request;

$pluginClient = new \Http\Client\Common\PluginClient(
  Http\Adapter\Guzzle6\Client::createWithConfig(['base_uri' => 'http://httpbin.org/']), [
    new \Http\Client\Common\Plugin\ErrorPlugin(),
  ]
);

// Initiate each request but do not block
$promises = [
  'image' => $pluginClient->sendAsyncRequest(new Request('GET', 'http://httpbin.org/status/404')),
  'png'   => $pluginClient->sendAsyncRequest(new Request('GET', 'http://httpbin.org/status/200')),
  'jpeg'  => $pluginClient->sendAsyncRequest(new Request('GET', 'http://httpbin.org/status/200')),
  'webp'  => $pluginClient->sendAsyncRequest(new Request('GET', 'http://httpbin.org/status/404')),
//  'delay' => $pluginClient->sendAsyncRequest(new Request('GET', 'http://httpbin.org/redirect/6')),
];

try {
  // Wait for the requests to complete, even if some of them fail
  $results = Promise\settle($promises)->wait();
}
catch (\Exception $e) {
  printf("Exception should not be thrown. %s\n", get_class($e));
}
exit(0);

Outputs: "Exception should not be thrown. Http\Client\Common\Exception\ClientErrorException".

Expected behaviour:

<?php

require_once "vendor/autoload.php";

use GuzzleHttp\Client;
use GuzzleHttp\Promise;
use GuzzleHttp\Psr7\Request;

$pluginClient = new \Http\Client\Common\PluginClient(
  new \Http\Client\Curl\Client(\Http\Discovery\MessageFactoryDiscovery::find(), \Http\Discovery\StreamFactoryDiscovery::find()), [
    new \Http\Client\Common\Plugin\ErrorPlugin()
  ]
);

// Initiate each request but do not block
$promises = [
  'image' => $pluginClient->sendAsyncRequest(new Request('GET', 'http://httpbin.org/status/404')),
  'png'   => $pluginClient->sendAsyncRequest(new Request('GET', 'http://httpbin.org/status/200')),
  'jpeg'  => $pluginClient->sendAsyncRequest(new Request('GET', 'http://httpbin.org/status/200')),
  'webp'  => $pluginClient->sendAsyncRequest(new Request('GET', 'http://httpbin.org/status/404')),
//  'delay' => $pluginClient->sendAsyncRequest(new Request('GET', 'http://httpbin.org/redirect/6')),
];

try {
  // Wait for the requests to complete, even if some of them fail
  $results = Promise\settle($promises)->wait();
}
catch (\Exception $e) {
  printf("Exception should not be thrown. %s\n", get_class($e));
}
exit(0);

Outputs: nothing, but $results contains proper results of promises.

[mxr576]

@sagikazarmark As maintainer of this library and Guzzle could you share your 2 cents please?

[sagikazarmark]

I will have to look into how Guzzle's settle works with HTTPlug promises. (Honestly I didn't expect them to work at all, but looks like they do thanks to https://github.com/guzzle/promises/blob/master/src/functions.php#L73 )

I suspect that somewhere there is a plain wait call which is indeed not an issue if the unwrapped value is a promise itself, which is never the case with HTTPlug promises. Your second example with the cURL client seems to be working though, so this is just a wild guess.

[mxr576]

@sagikazarmark Thanks for looking into this.

I suspect that somewhere there is a plain wait call which is indeed not an issue if the unwrapped value is a promise itself, which is never the case with HTTPlug promises.

Yeah, this is something that I could understand too about this problem. I tried plenty different solutions but none of them worked perfectly. Sometimes the only problem was that sendAsync() must return an Http\Promise\Promise but Guzzle's own Promise implementation returned. When I tried to wrap that and hide it Http\Promise then I bump into other problems.

Before I started to work on this I had asked in the issue queue of php-http/client-common before how async can be actually leveraged with Httplug. What is your suggestion Mark? Should I just try to use Promise\unwrap and forgot about Promise\settle? (TBH, I have not checked yet whether there is an another/similar issue with unwrap or not.)

https://github.com/php-http/client-common/issues/105

[sagikazarmark]

For now I would suggest calling the wait method of the HTTPlug promise.

As I said: the Guzzle promise functions are not designed to work with HTTPlug promises, so I don't see that as a valid expectation. I see it as a valid use case though, this might be something that we should improve in the future.

Also, keep in mind that HTTPlug does not provide any event loop like solution, like Guzzle does (kinda), so eventually you have to call wait at some point to make sure that it works with all client implementations.

Sometimes the only problem was that sendAsync() must return an Http\Promise\Promise but Guzzle's own Promise implementation returned

Can you provide some code to reproduce this behaviour? (TBH I'm quite certain that this should never happen)

If you need to utilize advanced Guzzle features (like the request pool), I would suggest you to use Guzzle though, hiding it behind your own transport abstraction layer fitting your needs.

Anyway, I would like to look into the Guzzle functions when I find the time.

[mxr576]

@sagikazarmark

Can you provide some code to reproduce this behaviour? (TBH I'm quite certain that this should never happen)

I haven't kept these non-working solutions. What I remember is that I tried to decorate the vanilla Guzzle Promise object returned by sendAsync() in the Guzzle6 adapter's own promise class (which implements \Http\Promise\Promise) when this happened. Unfortunately, Guzzle's promise implementation checks some places whether the returned promise is an instance of GuzzleHttp\Promise\PromiseInterface, ex.: https://github.com/guzzle/promises/blob/v1.3.1/src/Promise.php#L64

Also, it seems not everything is perfect with the Curl client either :-(

When this happens a logical exception is also thrown with the following error:

Cannot change a rejected promise to fulfilled
Cannot change a rejected promise to fulfilled

I am using the Error plugin as you can see and I think this is one of the reasons of this problem. Error plugin throws an exception for >= 400 HTTP codes. Guzzle considers 404 as fulfilled but error plugin rejects that with an exception.

[mxr576]

Maybe returning a \Http\Promise\RejectedPromise in Error handler plugin could be the "Guzzle Promise way" for this problem, but you can not do that because HttpPlugin plugin's onfulfilled function's parameter must be ResponseInterface and not a Promise.

TypeError: Argument 1 passed to Http\Client\Common\Plugin\HistoryPlugin::Http\Client\Common\Plugin\{closure}() must implement interface Psr\Http\Message\ResponseInterface, instance of Http\Promise\RejectedPromise given in /var/www/html/vendor/php-http/client-common/src/Plugin/HistoryPlugin.php on line 39

[mxr576]

Finally could knock together a POC for the CURL client so I reported it: https://github.com/php-http/curl-client/issues/39

As you can see, Error plugin is not needed to reproduce the issue.