Async Http Client #77

joelwurtz · 2015-10-26T14:02:42Z

See #74 and #75 discussion for a more detailed description.

This PR intend to add asynchronous support for httplug by adding a new interface HttpAsyncClient which allow to send an asynchronous request and return a promise for the response.

The current Promise interface follow specification of https://promisesaplus.com/ from an user point of view (there is no method for fulfilling or rejecting the promise, it should be up to the implementation user only care on how to handle the return and not on how to resolve the promise)

joelwurtz · 2015-10-26T17:21:19Z

Actually the onFulfilled / onRejected are a callable, WDYT if we replace them by an interface ?

(Would be easier to typehint and also not so a problem with inline implementation for the futur, as we will have anonymous class, and we can always wrap a defaut implementation which takes a callable)

sagikazarmark · 2015-10-26T18:40:05Z

Default implementation on the other hand causes to depend on the utils package.

joelwurtz · 2015-10-29T09:22:58Z

Hum right, we can stick with callable for the moment.

I think it is ready for review then, i don't see anything to add. I will maybe add some docs on the getState method as in my mind this method as the role to wait / check the underlying implementation (like a stream_select).

dbu · 2015-10-29T09:52:05Z

src/Promise.php

+     * If the callback is null it must not be called.
+     * It must be called after promise is rejected, with promise’s reason (Exception) as its first argument.
+     * It must not be called before promise is rejected.
+     * It must not be called more than once.


this looks like phpspec to me. maybe we can condense this a bit and make it more readable? and i would write it from the perspective of somebody using the promise, not the implementors.

the general description could state what is common:

If you do not care about one of the cases, you can set the corresponding callable to null

The callback will be called when the response resp. exception arrived and never more than once.

...

Agreed also, as i want to limit all these to the user point of view, this should be more in the implementation documentation

dbu · 2015-10-29T09:57:35Z

very cool!

in terms of usability, i wonder if this client could extend the normal HttpClient. when you can send async requests, you for sure can also send other requests (worst case waiting for the result of an async request in the client). or not? just trying to think how many if instanceof we want users of the library to do...

joelwurtz · 2015-10-29T10:03:38Z

Yes i was thinking the same thing, itreally depend on implementation as guzzle provide both mecanism we don't need to do that, but for React adapter i think we will provide also a "wait" mechanism for the synchronous sendRequest

sagikazarmark · 2015-10-29T11:12:27Z

I think interface inheritance does not really matter here.

dbu · 2015-10-29T12:19:16Z

yeah, actually a use case where you need both async and sync request seems not very obvious. guess we are fine like this, keeps things simpler.

so lets clean up the exception behaviour and callback documentation and then merge this. oh, and maybe build a doc PR that goes with this ;-)

sagikazarmark · 2015-10-29T12:56:40Z

👍

joelwurtz · 2015-10-29T20:25:56Z

Just updated but dot not merge it, i want to try react and guzzle6 implementations before submitting this, to be sure that this interface has all requirements. Also i will do the documentation PR linked to this.

sagikazarmark · 2015-10-29T20:51:58Z

Won't merge it until the WIP tag is included in the title.

joelwurtz · 2015-10-30T21:16:46Z

After looking at the Guzzle and React implementation for their async request i think we need a wait function to this interface.

As in real, expect with some "hacking" (like using the tick register handler of PHP), there is no way that our callback will be called magically if somehow we do not wait for the result to be available at some moment.

So IMO we must provide a wait function for the user when he needs the response of this request to be complete.

dbu · 2015-10-30T22:09:08Z

agreed. can we just use the guzzle impl in the guzzle adapter?

joelwurtz · 2015-10-30T22:34:12Z

We have to provide a decorator to not collide with their interface, but this is pretty straigthforward otherwise.

dbu · 2015-10-31T11:18:46Z

We have to provide a decorator to not collide with their interface, but
this is pretty straigthforward otherwise.

👍 decorator is more clean than extend to add the interface anyways.

sagikazarmark · 2015-10-31T19:03:32Z

src/Promise.php

+     * The callback will be called when the response or exception arrived and never more than once.
+     *
+     * @param callable $onFulfilled Called when a response will be available.
+     * @param callable $onRejected Called when an error happens.


Description indentation

sagikazarmark · 2015-10-31T19:21:15Z

Not sure if I completely get this async idea. Shouldn't calling the wait return the response or throw an exception?

joelwurtz · 2015-10-31T20:33:31Z

Guzzle does that by default but also provide a way to do like here (using the $unwrap parameter)

In our case, i prefer to have a limited ways of getting the response, (this also make implementation lighter).

But i really don't mind about one solution or the other.

dbu · 2015-11-01T20:29:24Z

src/Promise.php

+     *
+     * This function does not return a result, it simply wait for response or error
+     * of the request to be available, change the state of the promise and call one
+     * of the then callable.


lets add something like "When this method returns, the request has been resolved and the appropriate callable has terminated.

dbu · 2015-11-01T20:35:14Z

hm, i agree we only want one or the other way. not sure which is better though. when using the callable mechanism, not returning anything makes sense. when not using them, returning would make more sense.

but i guess the callable mechanism is really useful, particularly when there are several promises and we don't know which will be resolved first. (does that work with php? when i wait on one promise, if another one resolves while i wait, will the callable of the other promise be executed?)

if callables work that way, i would prefer that we promote the callable approach in the doc and have wait() not return anything. we should keep getError / getResponse in either case, to make it not unnecessary hard to access that information if desired (like write a callable to deposit the information somewhere...)

dbu · 2015-11-01T20:36:34Z

src/Promise.php

+     *
+     * @return Exception Exception Object only when the Promise is rejected.
+     */
+    public function getError();


why not getException?

joelwurtz · 2015-11-04T09:59:44Z

but i guess the callable mechanism is really useful, particularly when there are several promises and we don't know which will be resolved first. (does that work with php? when i wait on one promise, if another one resolves while i wait, will the callable of the other promise be executed?)

What do you mean by several promises ?

A promise for another sendAsyncRequest ?
Or a chained one like :

$promise
  ->then(function() ..., function() ...)
  ->then(function () ..., function () ...)
;

If it's a promise for another sendAsyncRequest, it will depend on the implementation, for guzzle i don't think so but for react certainly if we use a global event loop.

For the chained one it will call promises in the order of declaration.

By the way even if we return a Response / Exception in the wait method, the then callables will always be called as it is mandatory.

dbu · 2015-11-04T13:00:37Z

i was thinking about parallel async requests, yes. so lets not return anything in the wait and promote the callables way, but keep the getResponse / getException methods for people that want to work differently.

joelwurtz · 2015-11-04T22:44:48Z

So, this should be good to merge, after reviewing the Observable vs Promise discussion on the Promise RFC on fig group, i don't think this pattern is better or not (i find it very similar, see discussion on slack with @sagikazarmark).

… the promise

…t callback

dbu · 2015-11-05T07:35:37Z

src/Promise.php

+     * @return Exception Exception Object only when the Promise is rejected.
+     *
+     * If the exception is an instance of Http\Client\Exception\HttpException it will contain
+     * the response object with the status code and the http reason.


i think this should go above the @return line

dbu · 2015-11-05T07:40:19Z

how soon can we expect FIG to release a promise standard? if its reasonable to expect it by end of the year, i would delay support for async to 1.1 of HTTPlug to get their promises rather than define our own.

alternatively i could live with clearly stating that the Promise interface is to be considered experimental and will be replaced by the FIG one in a minor version change. or we just go 2.0 after 3 months... its hard to say i guess how long FIG will take. and we really want to wait with adopting their thing until its actually stable released. maybe a version 2.0 of HTTPlug in half a year is the way to go.

sagikazarmark · 2015-11-05T07:57:33Z

I don't think we can expect a standard this year.

What if we place the async stuff in a separate repository as @joelwurtz suggested. We can have it experimental until the Promise question is cleared, we can merge them afterwards. This way we would have a clear and super stable contract without experimental stuff.

dbu · 2015-11-05T08:00:26Z

okay, that makes sense to me. with a note that this will become deprecated and move to the HTTPlug main repo once PSR is released.

sagikazarmark · 2015-11-05T08:03:12Z

Sure. @joelwurtz I've setup httplug-async repo. Can you push content there? Keep in mind that it must require the httpluh repo because of exceptions.

joelwurtz · 2015-11-05T08:48:41Z

Sure, should i keep the same namespace or put everything under Http\Client\Async ?

sagikazarmark · 2015-11-05T08:49:14Z

I think you should keep it as it is only a temporary solution.

dbu · 2015-11-05T08:50:06Z

yeah, it technically works with composer to have several packages provide classes/interface in the same namespace

joelwurtz · 2015-11-05T09:09:29Z

done in https://github.com/php-http/httplug-async

dbu · 2015-11-05T09:53:08Z

hurray!

Fix Puli issue on PHP 5.6

dbu reviewed Oct 29, 2015
View reviewed changes

This was referenced Oct 31, 2015

Add test for async client php-http/client-integration-tests#10

Merged

Add async capability php-http/guzzle6-adapter#11

Merged

Add documentation for async client php-http/documentation#21

Merged

joelwurtz changed the title ~~[WIP] Async Http Client~~ Async Http Client Oct 31, 2015

sagikazarmark reviewed Oct 31, 2015
View reviewed changes

dbu reviewed Nov 1, 2015
View reviewed changes

joelwurtz added 10 commits November 5, 2015 00:14

Replace sendRequests with an async approach

d6b548c

Force callback to be set

9b7bbf2

Use specification of promises/a+

b6c5434

Replace "should" by "must"

5628eee

Add documentation about states, add method to get value and reason of…

1c569e7

… the promise

Add note about promises/a+

5a868bd

Fix typo on request <-> response

21e01c1

Fix documentation from an user point of view

f2af5cb

Use string for constant, add wait method, add some documentation abou…

fe7d30b

…t callback

Fixing phpdoc, rename getError to getException

55b159f

dbu reviewed Nov 5, 2015
View reviewed changes

joelwurtz closed this Nov 5, 2015

Nyholm pushed a commit to Nyholm/httplug that referenced this pull request Dec 26, 2019

Merge pull request php-http#77 from php-http/puli_fix

43c3140

Fix Puli issue on PHP 5.6

Async Http Client #77

Async Http Client #77

Uh oh!

Conversation

joelwurtz commented Oct 26, 2015

Uh oh!

joelwurtz commented Oct 26, 2015

Uh oh!

sagikazarmark commented Oct 26, 2015

Uh oh!

joelwurtz commented Oct 29, 2015

Uh oh!

dbu Oct 29, 2015

Choose a reason for hiding this comment

Uh oh!

sagikazarmark Oct 29, 2015

Choose a reason for hiding this comment

Uh oh!

joelwurtz Oct 29, 2015

Choose a reason for hiding this comment

Uh oh!

dbu commented Oct 29, 2015

Uh oh!

joelwurtz commented Oct 29, 2015

Uh oh!

sagikazarmark commented Oct 29, 2015

Uh oh!

dbu commented Oct 29, 2015

Uh oh!

sagikazarmark commented Oct 29, 2015

Uh oh!

joelwurtz commented Oct 29, 2015

Uh oh!

sagikazarmark commented Oct 29, 2015

Uh oh!

joelwurtz commented Oct 30, 2015

Uh oh!

dbu commented Oct 30, 2015 via email

Uh oh!

joelwurtz commented Oct 30, 2015

Uh oh!

dbu commented Oct 31, 2015

Uh oh!

sagikazarmark Oct 31, 2015

Choose a reason for hiding this comment

Uh oh!

sagikazarmark commented Oct 31, 2015

Uh oh!

joelwurtz commented Oct 31, 2015

Uh oh!

dbu Nov 1, 2015

Choose a reason for hiding this comment

Uh oh!

dbu commented Nov 1, 2015

Uh oh!

dbu Nov 1, 2015

Choose a reason for hiding this comment

Uh oh!

joelwurtz commented Nov 4, 2015

Uh oh!

dbu commented Nov 4, 2015

Uh oh!

joelwurtz commented Nov 4, 2015

Uh oh!

dbu Nov 5, 2015

Choose a reason for hiding this comment

Uh oh!

dbu commented Nov 5, 2015

Uh oh!

sagikazarmark commented Nov 5, 2015

Uh oh!

dbu commented Nov 5, 2015 via email

Uh oh!

sagikazarmark commented Nov 5, 2015

Uh oh!

joelwurtz commented Nov 5, 2015

Uh oh!

sagikazarmark commented Nov 5, 2015

Uh oh!

dbu commented Nov 5, 2015 via email