[WIP] Asynchronous sendAsync() immediately returns FutureResponse with blocking receive() method #466

Closed
wants to merge 6 commits into
from

Projects

None yet

6 participants

@naderman

Work in Progress: Not mergable yet, lots of cleanup todo, docs missing so far.

The basic idea of this PR is to make it possible to send off requests, then continue with some work in PHP while the remote server is responding, and to then process the response later in the script. This is made possible with new sendAsync() methods which return FutureResponse objects which contain a blocking receive method that waits for a response and returns it. The following snippet demonstrates the basic workflow:

// create $request as usual
$futureResponse = $request->sendAsync();
// do some complicated work that takes a while, aka napping
sleep(1);
$response = $futureResponse->receive();
// do something with the response
echo $response->getBody();

The current send() method continues to work as is, it internally calls sendAsync() and then immediately calls receive() on the future response and only returns the actual response.

Alternatively we could have FutureResponse implement the same Response interface and implement all calls to block until the response is received and then return the result of corresponding Response methods - either manually or via a magic __call method. I haven't looked at how this would interact with custom response classes yet. If we did this, we would not have to explicitly call sendAsync(), but this new behaviour would simply be the default behaviour of send(). All code using guzzle could remain the same and would automatically benefit from this change.

The underlying modification that made this possible is the split of perform() into performWrite() and performRead(). Consequently the CurlMultiInterface now requires a call to send() to be followed by a call to receive(), which blocks until a response is received.

Please let me know if the overall concept of how I'm implementing this is acceptable. I've been trying to keep the interface as backward compatible as possible for now. send() on both Client and Request continues to work as it did previously.

@amacneil
Contributor

Without knowing anything about how the curl async stuff works, would it be possible to return a promise that you could attach callbacks to, a la jQuery?

$futureResponse->done(function($response) {
    // do something with response
});

// code here executes immediately

That would be a more familiar pattern than simply blocking inline until the request is complete.

@naderman

There is no event loop in PHP by default so you would still have to have code that determines when your done function is actually called, so that kind of callback architecture really makes no sense for PHP.

@naderman

That is entirely unecessary and not what I am proposing here? I really don't understand why you would want to make guzzle depend on libevent for this.

@amacneil
Contributor

I was just wondering purely from an API point of view. If it's not possible or too difficult in PHP then it's no problem.

On Wed, Nov 13, 2013 at 3:38 AM, Nils Adermann notifications@github.com
wrote:

That is entirely unecessary here? I really don't understand why you would want to do that here?

Reply to this email directly or view it on GitHub:
#466 (comment)

@mtdowling
Member

Thanks for putting all this work into this PR. (sorry for the delay; I'm out of town right at AWS Re:Invent).

Here's some feedback:

  1. This would have to be profiled to ensure that this isn't introducing a lot of overhead over the common use case of sending a single request. I'd also want to ensure it doesn't significantly impact sending requests in parallel as it's currently done in Guzzle.
  2. Ideally a FutureResponse would decorate a Response, however, Response does not have an interface-- so that makes this more difficult. However, I'd still prefer FutureResponse to implement the full Response interface (possibly in a way that extends Response so that typehints still work).
  3. The async support in cURL is non-blocking and requires you to continuously call curl_multi_exec for cURL to send request data and receive response data. When you add a handle and call curl_multi_exec, cURL will do as little work as possible (because it's non-blocking) and then return. If there's data to upload, then it probably wont send much (or any) of that data until you call the blocking receive() method of the Response. If other requests are added to the CurlMulti object as an async request, then when a single async response is told to block, it will have to wait for all of the other async requests to complete before yielding control back to the point in the script that called the receive method of the single response. Based on this, this PR seems to basically implement a queue of requests to eventually send, but pushes the sending of the request off onto cURL. When one of the Responses is eventually told to block, a user is basically using the same implementation we currently have for sending requests in parallel.

I could be wrong about number 3 though. I wonder if there's a way to run a tcpdump along with executing some test cases to show if this actually has a benefit. Maybe performance testing against the node.js test server that ships with Guzzle would be beneficial as well.

@mtdowling
Member

I'm going to go ahead and close this issue. I think this is a pretty big change for the 3.x branch and as I stated before, doesn't add functionality beyond creating a queue of requests that is managed internally by the CurlMulti object rather than an external pool. That said, this type of future response functionality would be much easier to add to Guzzle 4.0 and would be a pretty cool third-party addon.

@mtdowling mtdowling closed this Mar 12, 2014
@naderman

Well I'm still maintaining this in a fork of mine, and I did mean to address your comments, just never found the time to do so. This still significantly speeds up a lot of uses of guzzle without adding any incompatability - not sure why you wouldn't want to make this the default behaviour for 4.0 to begin with?

@mtdowling
Member

I'm trying to simplify public interfaces in Guzzle 4 as much as possible. I think that making future responses the default behavior adds too much complexity for common use cases. People who need to send multiple requests as efficiently as possible can use the parallel requests feature of Guzzle. This feature has been optimized quite a bit in Guzzle 4 to use a rolling queue rather than batching.

@naderman

This feature is not about sending multiple requests. In fact it is extremely useful for sending individual requests. When implemented to behave the same as a regular response, there is no difference for a user, except the program runs faster if anything is executed between the send() call and the first access of any response data.

@mtdowling
Member

Like I said earlier, I don't think curl works this way. You need to call curl_multi_exec over and over to transfer data. I'd like to see benchmarks that show that this actually speeds things up. Either way, this adds more complexity than I'd like for users and the implementation.

One of the things I hope to achieve with 4.0 is that developers can easily create custom HTTP adapters for specific needs. For example, I'm going to work on a pthreads adapter that uses a thread pool to transfer HTTP requests. I'll probably also create a socket based adapter (likely by wrapping something like React or Artax). Having a future adapter would another cool addition to the pluggable adapters.

@mtdowling
Member

I've implemented a similar approach to clean up Guzzle's API to use a single adapter that is just a callable function. In order to implement parallel requests, I've added FutureResponse objects that are only sent over the wire when a future response is used. See: #810.

@naderman
naderman commented Sep 7, 2014

Well I'm happy to hear you added FutureResponse objects. However only sending requests once you access the object entirely defeats the purpose of having the requests being worked on in parallel to executing code. You should be sending requests right away and only block to wait on responses when they are accessed.

@mtdowling
Member

There's no mandate that a request can't be started immediately. It just depends on the adapter. Sending requests immediately with cURL can result in poor performance when dealing with low latency servers (e.g., localhost). I've benchmarked this and found a 10-20% increase in speed when you just add the handles and only send them in parallel when one of the requests needs a response. Because cURL doesn't keep sending requests to the remote server or do anything without you calling curl_exec() over and over, I don't see any benefit in calling curl_exec() each time a handle is added.

That said, an adapter could be created using something else that opens requests immediately. Would be cool to have one that sends requests in a separate thread via pthreads and truly sends in the background.

@mtdowling
Member

I just played around with sending requests to an external site, calling curl_exec immediately and compared against waiting until a response is used. The results are pretty much equivalent. When sending to a localhost sever, I'm still seeing a 10-20% reduction in performance. Do you have an example of where calling curl_exec immediately would result in better performance?

@naderman
naderman commented Sep 8, 2014

This really comes down to the part of libcurl that you didn't believe/understand that I made use of in my PR here.

https://github.com/guzzle/guzzle-ring/blob/master/src/Client/CurlMultiAdapter.php#L123

do {
    $mrc = curl_multi_exec($this->mh, $active);
} while ($mrc === CURLM_CALL_MULTI_PERFORM);

The first time you call this loop it will exit the loop once all local (request) data has been sent. You then need to call curl_multi_select to make curl wait on incoming data. It will return as soon as there is any data on any sockets curl has open. You then call the do/while loop again for curl to handle incoming data and repeat this process until done. You do this correctly but in a closed loop.

For performance benefits you need to send requests, can then execute other PHP code while the remote server is working and then wait for the response when the future is accessed. To do so you must run the above do/while loop once until it exits. And then start the curl_multi_select followed by repeated calls do the do/while loop as implemented in your execute() method only once the Future is accessed.

@naderman
naderman commented Sep 8, 2014

Basically something along the lines of:

    // to be called right away
    private function startExecute()
    {
        do {
            $mrc = curl_multi_exec($this->mh, $active);
        } while ($mrc === CURLM_CALL_MULTI_PERFORM);
        $this->processMessages();
        $this->active = $active;
    }

    // to be called when the future response is accessed
    private function finishExecute()
    {
        $active = $this->active;
        do {
            if ($active &&
                curl_multi_select($this->mh, $this->selectTimeout) === -1
            ) {
                // Perform a usleep if a select returns -1.
                // See: https://bugs.php.net/bug.php?id=61141
                usleep(250);
            }

            do {
                $mrc = curl_multi_exec($this->mh, $active);
            } while ($mrc === CURLM_CALL_MULTI_PERFORM);

            $this->processMessages();
        } while ($this->handles || $active);
    }
@mtdowling
Member

I'm seeing degraded performance in some cases which is probably caused by the fact that it blocks on each added request without allowing a large pool of requests to build up.

This runs in a loop without allowing a pool of requests to build up:

do {
    $mrc = curl_multi_exec($this->mh, $active);
} while ($mrc === CURLM_CALL_MULTI_PERFORM);

For example, with your suggested changes, sending a bunch of GET requests to a local server provides a slightly improved performance over waiting for them to queue up. However, sending PUT requests in parallel with 100k bodies results in a 10% degredation. Maybe there's something I'm missing? Maybe this cost in performance is worth it? What do you think?

diff --git a/src/Client/CurlMultiAdapter.php b/src/Client/CurlMultiAdapter.php
index 8af0241..44af1db 100644
--- a/src/Client/CurlMultiAdapter.php
+++ b/src/Client/CurlMultiAdapter.php
@@ -21,6 +21,9 @@ class CurlMultiAdapter
     /** @var int */
     private $selectTimeout;

+    /** @var bool */
+    private $active;
+
     private $handles = [];
     private $processed = [];
     private $futures = [];
@@ -109,6 +112,10 @@ class CurlMultiAdapter
     {
         $this->handles[(int) $handle] = [$handle, &$request, []];
         curl_multi_add_handle($this->mh, $handle);
+        do {
+            $mrc = curl_multi_exec($this->mh, $this->active);
+        } while ($mrc === CURLM_CALL_MULTI_PERFORM);
+        $this->processMessages();
     }

     private function removeProcessed($id)
@@ -123,21 +130,18 @@ class CurlMultiAdapter
     private function execute()
     {
         do {
-            do {
-                $mrc = curl_multi_exec($this->mh, $active);
-            } while ($mrc === CURLM_CALL_MULTI_PERFORM);
-
-            $this->processMessages();
-
-            if ($active &&
+            if ($this->active &&
                 curl_multi_select($this->mh, $this->selectTimeout) === -1
             ) {
                 // Perform a usleep if a select returns -1.
                 // See: https://bugs.php.net/bug.php?id=61141
                 usleep(250);
             }
-
-        } while ($this->handles || $active);
+            do {
+                $mrc = curl_multi_exec($this->mh, $this->active);
+            } while ($mrc === CURLM_CALL_MULTI_PERFORM);
+            $this->processMessages();
+        } while ($this->handles || $this->active);
     }

     private function processMessages()
@naderman
naderman commented Sep 8, 2014

You are looking at an entirely different use case from what this has an advantage for, and are measuring something different from what matters to most programmers.

This makes a huge difference if you send HTTP requests to a remote server that either takes a while to process before it sends a response or has a lot of latency. You can see what the difference is in this example I made way back when i created this PR: https://gist.github.com/naderman/7267028

As explained in the paragraph above the code, you end up running your own PHP code in parallel to whatever is happening on the server. So you get a 100% performance improvement over whatever the server processing time and response sending time is. However this time is gained by running code in parallel. Obviously the overall speed of the response coming back is still the same, but you get to do computations in your PHP code for the entire time that the request is being processed on the server. This is parallelization and not measurable in a serialized measure. I'm really not sure how else to explain this anymore.

@pierrejoye

I have to totally agree with @naderman here. The concept of async requests allow exactly what he describes here. Doing only requests may not show any gain simply because nothing else is done during the requests time. Imagine 50+ms per request, that stacks up a lot and gives you plenty of times to do other things while the requests are processing. This is a key concept for fast client/server communication.

@mtdowling
Member

@naderman You're right. I was trying to solve a different, slightly related problem. What you want to do is send a request and not block for the response. What I was trying to do was send a bunch of requests in parallel as quickly as possible. I see the value in what you're saying, and I'll make that change. In order to accommodate sending a large number of requests in parallel as quickly as possible (i.e., using Guzzle's sendAll() method), I think I'll look into adding a curl specific option to requests to prevent the exec from happening up front, but rather happen only when the queue is full. This should be the best of both worlds using the same implementation. Let me know if that doesn't make sense.

@naderman
naderman commented Sep 8, 2014

@mtdowling Thank you so much! Yes I understand that making request sending itself faster may be important to some users too, but as long as there is an option to pick either behavior that'll be fine :-)

@jakoch not sure what you are on about, this is a discussion about adding non-blocking I/O, you seem to have missed the point entirely

@nikita2206

By the way, about calling curl_multi_exec over and over again, you can delay these calls to the moment when you'll really need the response, at this point server possibly already sent all this data to the socket so when you will execute curl_multi_exec you will just fetch it from your OS's local buffer. That's why proposed strategy can be faster :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment