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

Issue 55 curl multirunner burn cpu #56

Open
wants to merge 2 commits into
base: 2.x
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 5 additions & 1 deletion composer.json
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,9 @@
],
"prefer-stable": true,
"minimum-stability": "dev",
"config": {
"secure-http": true
},
"require": {
"php": "^7.1",
"ext-curl": "*",
Expand All @@ -30,7 +33,8 @@
"guzzlehttp/psr7": "^1.0",
"php-http/client-integration-tests": "^2.0",
"phpunit/phpunit": "^7.5",
"zendframework/zend-diactoros": "^2.0"
"zendframework/zend-diactoros": "^2.0",
"donatj/mock-webserver": "^2.0"
},
"autoload": {
"psr-4": {
Expand Down
4 changes: 4 additions & 0 deletions phpunit.xml.dist
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,10 @@
<directory>tests/Unit</directory>
</testsuite>

<testsuite name="Integration">
<directory>tests/Integration</directory>
</testsuite>

<testsuite name="Functional">
<directory>tests/Functional</directory>
<!-- Exclude till https://github.com/php-http/message/issues/105 resolved. -->
Expand Down
24 changes: 19 additions & 5 deletions src/MultiRunner.php
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,11 @@
*/
class MultiRunner
{
/**
* Timeout for curl_multi_select in seconds.
*/
private const SELECT_TIMEOUT = 0.1;

/**
* cURL multi handle.
*
Expand Down Expand Up @@ -84,18 +89,27 @@ public function remove(PromiseCore $core): void
public function wait(PromiseCore $targetCore = null): void
{
do {
$status = curl_multi_exec($this->multiHandle, $active);
if (curl_multi_select($this->multiHandle, self::SELECT_TIMEOUT) === -1) {
// See https://bugs.php.net/bug.php?id=61141
usleep(250);
dbu marked this conversation as resolved.
Show resolved Hide resolved
}

do {
$status = curl_multi_exec($this->multiHandle, $active);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this now again busy waiting? should we repeat the curl_multi_select in each loop?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not pretty sure. Most examples I've found are like this https://bugs.php.net/bug.php?id=61141#1348321490

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

its again the same as guzzle does, which is most likely what the symfony http client is using, so i'd hope its the right thing.

// TODO CURLM_CALL_MULTI_PERFORM never returned since cURL 7.20.
} while ($status === CURLM_CALL_MULTI_PERFORM);

$info = curl_multi_info_read($this->multiHandle);
if (false !== $info) {
if ($info !== false) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please don't revert yoda style comparisons.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because it has nothing to do with the change at hand.

i would have expected the code style checker to complain, but seems we don't have a rule for this activated.

$core = $this->findCoreByHandle($info['handle']);

if (null === $core) {
if ($core === null) {
// We have no promise for this handle. Drop it.
curl_multi_remove_handle($this->multiHandle, $info['handle']);
continue;
}

if (CURLE_OK === $info['result']) {
if ($info['result'] === CURLM_OK) {
$core->fulfill();
} else {
$error = curl_error($core->getHandle());
Expand All @@ -108,7 +122,7 @@ public function wait(PromiseCore $targetCore = null): void
return;
}
}
} while ($status === CURLM_CALL_MULTI_PERFORM || $active);
} while ($active);
}

/**
Expand Down
91 changes: 91 additions & 0 deletions tests/Integration/MultiRunnerTest.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
<?php

declare(strict_types=1);

namespace Http\Client\Curl\Tests\Integration;

use donatj\MockWebServer\MockWebServer;
use Http\Client\Curl\MultiRunner;
use Http\Client\Curl\PromiseCore;
use PHPUnit\Framework\TestCase;

/**
* Tests for Http\Client\Curl\MultiRunner.
*
* @covers \Http\Client\Curl\MultiRunner
*/
class MultiRunnerTest extends TestCase
{
/**
* Test HTTP server.
*
* @var MockWebServer
*/
private static $server;

/**
* Prepare environment for all tests.
*/
public static function setUpBeforeClass(): void
{
parent::setUpBeforeClass();

self::$server = new MockWebServer();
self::$server->start();
}

/**
* Cleanup environment after all tests.
*/
public static function tearDownAfterClass(): void
{
self::$server->stop();

parent::tearDownAfterClass();
}

public function testWait(): void
{
$runner = new MultiRunner();

$handle = $this->createCurlHandle('/');
$core1 = $this->createConfiguredMock(PromiseCore::class, ['getHandle' => $handle]);
$core1
->expects(self::once())
->method('fulfill');

$handle = $this->createCurlHandle('/');
$core2 = $this->createConfiguredMock(PromiseCore::class, ['getHandle' => $handle]);
$core2
->expects(self::once())
->method('fulfill');

$runner->add($core1);
$runner->add($core2);

$runner->wait($core1);
$runner->wait($core2);
}

/**
* Create cURL handle with given parameters.
*
* @param string $url Request URL relative to server root.
*
* @return resource
*/
private function createCurlHandle(string $url)
{
$handle = curl_init();
self::assertNotFalse($handle);

curl_setopt_array(
$handle,
[
CURLOPT_URL => self::$server->getServerRoot() . $url
]
);

return $handle;
}
}