Do not use up retries on requests which timed out during a prompt
Nothing drives the transfers while tick() is blocked, e.g. by a prompt for credentials, so requests which are still connecting time out with curl error 28. Since #12662 these are retried, but each retry counts like any other, so a few prompts can use up all of them, and parallel requests to a private host which timed out that way abort with "Invalid credentials" once the first of them got credentials, as AuthHelper only reuses those for a request which was not retried yet. tick() now keeps track of how long it was blocked between two ticks, and a request which spent most of its time in that gap is restarted without counting towards maxRetries. Fixes #12402
This commit is contained in:
1 parent
44656d3be2
commit
17d32b7184
2 files changed
+52
No files matched your search
@@ -69,6 +69,12 @@ class CurlDownloader
|
|||||||
* @var array<string, array{statusCode: int, announced: bool, until: float}>
|
* @var array<string, array{statusCode: int, announced: bool, until: float}>
|
||||||
*/
|
*/
|
||||||
private $retryAfterOrigins = [];
|
private $retryAfterOrigins = [];
|
||||||
|
/**
|
||||||
|
* When tick() last waited on the transfers, they are not driven again until the next tick(), which a prompt can hold up
|
||||||
|
*
|
||||||
|
* @var float|null
|
||||||
|
*/
|
||||||
|
private $lastSelectAt = null;
|
||||||
/** @var IOInterface */
|
/** @var IOInterface */
|
||||||
private $io;
|
private $io;
|
||||||
/** @var Config */
|
/** @var Config */
|
||||||
@@ -397,12 +403,14 @@ class CurlDownloader
|
|||||||
return;
|
return;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
$blockedFor = null !== $this->lastSelectAt ? microtime(true) - $this->lastSelectAt : 0.0;
|
||||||
$active = true;
|
$active = true;
|
||||||
$this->checkCurlResult(curl_multi_exec($this->multiHandle, $active));
|
$this->checkCurlResult(curl_multi_exec($this->multiHandle, $active));
|
||||||
if (-1 === curl_multi_select($this->multiHandle, $this->getSelectTimeout())) {
|
if (-1 === curl_multi_select($this->multiHandle, $this->getSelectTimeout())) {
|
||||||
// sleep in case select returns -1 as it can happen on old php versions or some platforms where curl does not manage to do the select
|
// sleep in case select returns -1 as it can happen on old php versions or some platforms where curl does not manage to do the select
|
||||||
usleep(150);
|
usleep(150);
|
||||||
}
|
}
|
||||||
|
$this->lastSelectAt = microtime(true);
|
||||||
|
|
||||||
while ($progress = curl_multi_info_read($this->multiHandle)) {
|
while ($progress = curl_multi_info_read($this->multiHandle)) {
|
||||||
$curlHandle = $progress['handle'];
|
$curlHandle = $progress['handle'];
|
||||||
@@ -437,6 +445,18 @@ class CurlDownloader
|
|||||||
}
|
}
|
||||||
$progress['error_code'] = $errno;
|
$progress['error_code'] = $errno;
|
||||||
|
|
||||||
|
// a request which spent most of its time blocked between two ticks, e.g. by a prompt for credentials, did not time out
|
||||||
|
// on its own, so it is retried without counting towards maxRetries, see https://github.com/composer/composer/issues/12402
|
||||||
|
if (
|
||||||
|
$errno === 28 /* CURLE_OPERATION_TIMEDOUT */
|
||||||
|
&& $blockedFor > $progress['total_time'] / 2
|
||||||
|
&& (!isset($job['options']['http']['method']) || $job['options']['http']['method'] === 'GET')
|
||||||
|
) {
|
||||||
|
$this->io->writeError('Retrying ' . Url::sanitize($job['url']) . ' due to curl error 28 while the download loop was blocked', true, IOInterface::DEBUG);
|
||||||
|
$this->restartJob($job, $job['url']);
|
||||||
|
continue;
|
||||||
|
}
|
||||||
|
|
||||||
if (
|
if (
|
||||||
(!isset($job['options']['http']['method']) || $job['options']['http']['method'] === 'GET')
|
(!isset($job['options']['http']['method']) || $job['options']['http']['method'] === 'GET')
|
||||||
&& (
|
&& (
|
||||||
|
|||||||
@@ -200,6 +200,38 @@ class CurlDownloaderTest extends TestCase
|
|||||||
self::assertSame([], $this->readProperty($downloader, 'jobs'), 'a cancelled request must not be sent again');
|
self::assertSame([], $this->readProperty($downloader, 'jobs'), 'a cancelled request must not be sent again');
|
||||||
}
|
}
|
||||||
|
|
||||||
|
public function testTimeoutWhileTheLoopWasBlockedDoesNotUseUpARetry(): void
|
||||||
|
{
|
||||||
|
// the kernel accepts the connection but nothing ever answers, so the request can only time out
|
||||||
|
$server = stream_socket_server('tcp://127.0.0.1:0');
|
||||||
|
self::assertIsResource($server);
|
||||||
|
$address = stream_socket_get_name($server, false);
|
||||||
|
self::assertIsString($address);
|
||||||
|
|
||||||
|
$downloader = $this->createDownloader();
|
||||||
|
$failure = null;
|
||||||
|
// a request which has no retries left, e.g. after timing out during earlier prompts
|
||||||
|
self::callPrivate($downloader, 'initDownload', static function (): void {
|
||||||
|
}, static function (\Exception $e) use (&$failure): void {
|
||||||
|
$failure = $e->getMessage();
|
||||||
|
}, $address, 'http://'.$address.'/packages.json', ['http' => ['timeout' => 1]], null, ['retries' => 3]);
|
||||||
|
|
||||||
|
$downloader->tick();
|
||||||
|
// a prompt for credentials keeps tick() from being called for longer than the request may take
|
||||||
|
usleep(1500000);
|
||||||
|
$downloader->tick();
|
||||||
|
|
||||||
|
self::assertNull($failure);
|
||||||
|
$jobs = $this->readProperty($downloader, 'jobs');
|
||||||
|
self::assertIsArray($jobs);
|
||||||
|
self::assertCount(1, $jobs, 'the request must have been sent again');
|
||||||
|
|
||||||
|
foreach (array_keys($jobs) as $id) {
|
||||||
|
$downloader->abortRequest((int) $id);
|
||||||
|
}
|
||||||
|
fclose($server);
|
||||||
|
}
|
||||||
|
|
||||||
public function testRetryWaitsForTheLatestRetryAfterOfItsOrigin(): void
|
public function testRetryWaitsForTheLatestRetryAfterOfItsOrigin(): void
|
||||||
{
|
{
|
||||||
$io = new BufferIO();
|
$io = new BufferIO();
|
||||||
|
|||||||
Reference in new issue
Block a user