Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MultiCurl::proceed is now blocking #352

Closed
lyrixx opened this issue Nov 18, 2018 · 8 comments · Fixed by #356
Closed

MultiCurl::proceed is now blocking #352

lyrixx opened this issue Nov 18, 2018 · 8 comments · Fixed by #356

Comments

@lyrixx
Copy link
Contributor

lyrixx commented Nov 18, 2018

Before version 1 (beta) MultiCurl::proceed() was non-blocking, and now it's a blocking call.
It waits until all requests finish.

<?php

use Buzz\Client\MultiCurl;
use Nyholm\Psr7\Factory\MessageFactory;
use Nyholm\Psr7\Factory\Psr17Factory;
use Nyholm\Psr7\Request;

require __DIR__.'/vendor/autoload.php';


$client = new MultiCurl(new Psr17Factory());

 $client->sendAsyncRequest($a = new Request('GET', 'http://httpbin.org/delay/1'), ['callback' => function($request, $response, $exception) {
    dump('A');
    dump(time() - $_SERVER['REQUEST_TIME_FLOAT']);
    // echo $response->getBody()->getContents();
}]);

 $client->sendAsyncRequest($a = new Request('GET', 'http://httpbin.org/delay/2'), ['callback' => function($request, $response, $exception) {
    dump('B');
    dump(time() - $_SERVER['REQUEST_TIME_FLOAT']);
    // echo $response->getBody()->getContents();
}]);
 $client->sendAsyncRequest($a = new Request('GET', 'http://httpbin.org/delay/3'), ['callback' => function($request, $response, $exception) {
    dump('C');
    dump(time() - $_SERVER['REQUEST_TIME_FLOAT']);
    // echo $response->getBody()->getContents();
}]);

while (true) {
    $client->proceed();
    echo '.';
    usleep(100000);
}
@Nyholm
Copy link
Collaborator

Nyholm commented Dec 27, 2018

Thank you for this issue.

I've spent a few hours yesterday working on this. Currently, proceed works as flush. This is indeed breaking change.

However, can you explain the use case for a non-blocking proceed function?

I will document the methods better and resolve this.

@lyrixx
Copy link
Contributor Author

lyrixx commented Dec 27, 2018

I will try to explain what I do with multi-curl:
I have built a Crawler that rely on AMQP, MultiCurl and other things.
Basically, I have a worker that grab URLs from AMQP. As soon as I fetch an URL, I init the HTTP Request (if I have enough room in my "HTTP Queue" (it's just an SplObjectStorage with a $maxItem property))
When the queue is full, I proceed the HTTP Queue to see if some request have finished.
If it's the case, I analyse the response(s) and so I free a place(s) for a next AMQP message(s) (it will create a new HTTP Request(s) that will be placed in the HTTP Queue.

Here is an extract of what I did (I removed some code, but the main idea is here):

while (true) {
    // Listen for AMQP message: trigger HTTP request for them
    while (!$httpQueue->isFull() && false !== $amqpEnvelope = $amqpQueue->get()) {
        $resource = $this->startMessage($amqpEnvelope, $httpQueue, $crawl);

        $httpQueue->add($resource, [
            'amqpEnvelope' => $amqpEnvelope,
            'amqpQueue' => $amqpQueue,
        ]);
    }

    // Check if HTTP request are finished
    $this->crawler->tick(); // Will call $buzz->proceed();

    // Process finished HTTP request
    foreach ($httpQueue->popFinished() as [$resource, $data]) {
        $this->processResponse($resource, $data['amqpEnvelope'], $data['amqpQueue']);
    }
}

As you can guess, I do not want to wait until all response are available. As soon as I get a response after the tick I proceed it and free a slot in the HTTP Queue.

With this workflow, I get really high performance, and almost everything is async

@Nyholm
Copy link
Collaborator

Nyholm commented Dec 27, 2018

Thank you

@lyrixx
Copy link
Contributor Author

lyrixx commented Dec 27, 2018

@Nyholm BTW, I think I will have to use another HTTP Client because I have very specific needs (as you can see) and because of this issue and #351

Do you know if there is something existing that could fit my use case? I think I will have to implement a new one :/

@Nyholm
Copy link
Collaborator

Nyholm commented Dec 27, 2018

I will fix these issues of yours this week. If you give up Buzz or not is irrelevant. =)

Do you know if there is something existing that could fit my use case?

Hm.. I dont think php-http/curl-client or Guzzle have a similar proceed() function. Maybe https://github.com/clue/reactphp-buzz could be worth looking into.

@Nyholm
Copy link
Collaborator

Nyholm commented Dec 27, 2018

I've got a fix for your already. See the diff: https://github.com/kriswallsmith/Buzz/pull/356/files?utf8=%E2%9C%93&diff=split&w=1#diff-f07f8f3af40548280b3046caf5c568bfL119

It is super simple to provide fixes when you done half the work and written test code. Thank you!

@Nyholm
Copy link
Collaborator

Nyholm commented Dec 27, 2018

Can you try master to see if it is working?

If so, I'll tag beta2.

@lyrixx
Copy link
Contributor Author

lyrixx commented Dec 27, 2018

sure. I do that :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants