Promise\settle() is not working properly about guzzle6-adapter HOT 8 OPEN

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.

from guzzle6-adapter.

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

from guzzle6-adapter.

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.

from guzzle6-adapter.

@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.)

php-http/client-common#105

from guzzle6-adapter.

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.

from guzzle6-adapter.

@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.

from guzzle6-adapter.

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

from guzzle6-adapter.

Finally could knock together a POC for the CURL client so I reported it: php-http/curl-client#39

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

from guzzle6-adapter.