-
Notifications
You must be signed in to change notification settings - Fork 28
Issue 55 curl multirunner burn cpu #56
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
base: 2.x
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,6 +14,11 @@ | |
*/ | ||
class MultiRunner | ||
{ | ||
/** | ||
* Timeout for curl_multi_select in seconds. | ||
*/ | ||
private const SELECT_TIMEOUT = 0.1; | ||
|
||
/** | ||
* cURL multi handle. | ||
* | ||
|
@@ -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); | ||
} | ||
|
||
do { | ||
$status = curl_multi_exec($this->multiHandle, $active); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. please don't revert yoda style comparisons. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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()); | ||
|
@@ -108,7 +122,7 @@ public function wait(PromiseCore $targetCore = null): void | |
return; | ||
} | ||
} | ||
} while ($status === CURLM_CALL_MULTI_PERFORM || $active); | ||
} while ($active); | ||
} | ||
|
||
/** | ||
|
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; | ||
} | ||
} |
Uh oh!
There was an error while loading. Please reload this page.