[RFC] Replace sendRequests with an async approach

joelwurtz commented 9 years ago

By working on a PoC of the Plugin system and some of the adapters, the sendRequests (multiple aproach) add a great complexity to all of this:

Actual problems

On the plugin system

First we start on thinking on a middleware chain (like stackphp.com) , but having the sendRequests method will imply a duplication on the execution (for each plugin we have to iterate over the requests, and the response, and futhermore we need to be extremly careful for the indexation of the reponse because of immutabilty see https://github.com/php-http/documentation/issues/11)

We then introduce the concept of having a pipeline chaining for this plugin, which is the concept i begin to implement in https://github.com/joelwurtz/plugin-client.

But with this approach, when trying to add the status code in error to exception plugin, i needed to update the transformResponse process, by adding the request to it which feels not right and not needed for all the plugins except this one (see https://github.com/joelwurtz/plugin-client/commit/d9d1b0d0ea2e189de90084788d749671281c5cc8#diff-3c253bd0a84a7e12f416110d65e05351R15)

Also the Retry plugin would not be possible as we need to encapsulate the sendRequest with a try / catch which is impossible with a pipeline, so we are back to the middleware (see the discussion on slack with @sagikazarmark), but would imply to handle the complexity for the sendRequests method on each plugin :/

On the adapter

Implementing the sendRequests method, on Guzzle6 adapter by exemple, indue to have all the requests / response in memory, which can be heavy on the system when there is many requests, or requests with large stream (even if this can somehow "handled").

Also as we have to stick with the BatchResult interface, we have for the moment, in all the adapter a dependence on the utils package for the implementation, as there is no storage system in Guzzle, neither in React as they all used a Promise system for the multiple request system.

Proposed solution

Move sendRequests method to an Implementation

sendRequests, is IMO a simple method, which simply take an array of requests and return an array of response. This can be easily done in an implementation which take an HttpClient and pipeline this array of requests and return a BatchResult, doing this will make HttpClient much easier and also the middleware solution would not suffer of implementing this method and all the complexity that comes with, as the batch system would be on an upper level.

Parallelism

One of the disavantage of doing this, is that we will drop support for library that can sends multiple request in parallel.

However sending requets in parallel, is just sending multiple request in an asychronous way.

Async Client

To support this, i propose a new interface: HttpAsyncClient which only send a RequestInterface object and return a Promise instead of the ReponseInterface, to support async client (guzzle5, guzzle6 and react)

The multiple requests client can then also accept a HttpAsyncClient and use it for sending request instead of the HttpClient.

Another advantage, is that it will allow to send one request in an asynchronous way, for the moment the only way to do that is to have the knowledge that the implementation send multiple requests in an asychronous way and use the sendRequests method with only one request:

$client->sendRequests([$request]);

For the plugin system, it would be also easier, IMO, to handle a Promise rather than a batch result Also, it would be possible to provide an abstract implementation to handle the middleware chaining for both type of client (HttpClient and HttpAsyncClient), so plugin can only focus on the transformation of the response / request system rather than the logic of the sendRequests method.

For adapter sending parallels / async requests by using a Promise system (react / guzzle), it would be easy to wrap their Promise with our own implementation, this will imply no storage on our end, and then no memory problem.

Final word

I don't think we should add this Promise / Async system in the upcoming alpha, as it's too early.

However IMO we should drop the sendRequests from the interface and add it in an implementation under the utils library.

Then in a another release we can easily add the parralelism / async in a new interface without breaking stuff (so we can release this in a 1.1 / 1.2 ... not in a 2.0).

This would also make things easier and focus more on the plugin system / adapter implementation which is the real value of php-http.

Icing on the cake

With the HttpAsyncClient we could also provide a new virtual package : php-http/async-client-implementation.

So if a library is doing some heavy works on http and want to force their users to use an async client it would be possible, also user will have a real view on which implementation can do asynchronous request or not.