Skip to content

[HttpClient] Fix early cleanup of pushed HTTP/2 responses #34554

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Nov 26, 2019
Merged

[HttpClient] Fix early cleanup of pushed HTTP/2 responses #34554

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions .travis.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,12 @@ before_install:
(cd php-$MIN_PHP && ./configure --enable-sigchild --enable-pcntl && make -j2)
fi

- |
# Install vulcain
wget https://github.com/symfony/binary-utils/releases/download/v0.1/vulcain_0.1.3_Linux_x86_64.tar.gz -O - | tar xz
sudo mv vulcain /usr/local/bin
docker pull php:7.3-alpine

- |
# php.ini configuration
for PHP in $TRAVIS_PHP_VERSION $php_extra; do
Expand Down Expand Up @@ -307,8 +313,14 @@ install:
PHPUNIT_X="$PHPUNIT_X,legacy"
fi

if [[ $PHP = ${MIN_PHP%.*} ]]; then
tfold src/Symfony/Component/HttpClient.h2push docker run -it --rm -v $(pwd):/app -v /usr/local/bin/vulcain:/usr/local/bin/vulcain -w /app php:7.3-alpine ./phpunit src/Symfony/Component/HttpClient/Tests/CurlHttpClientTest.php --filter testHttp2Push
fi

echo "$COMPONENTS" | parallel --gnu "tfold {} $PHPUNIT_X {}"

tfold src/Symfony/Component/Console.tty $PHPUNIT --group tty

if [[ $PHP = ${MIN_PHP%.*} ]]; then
export PHP=$MIN_PHP
tfold src/Symfony/Component/Process.sigchild SYMFONY_DEPRECATIONS_HELPER=weak php-$MIN_PHP/sapi/cli/php ./phpunit --colors=always src/Symfony/Component/Process/
Expand Down
30 changes: 22 additions & 8 deletions src/Symfony/Component/HttpClient/CurlHttpClient.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
use Symfony\Contracts\HttpClient\HttpClientInterface;
use Symfony\Contracts\HttpClient\ResponseInterface;
use Symfony\Contracts\HttpClient\ResponseStreamInterface;
use Symfony\Contracts\Service\ResetInterface;

/**
* A performant implementation of the HttpClientInterface contracts based on the curl extension.
Expand All @@ -32,7 +33,7 @@
*
* @author Nicolas Grekas <p@tchwork.com>
*/
final class CurlHttpClient implements HttpClientInterface, LoggerAwareInterface
final class CurlHttpClient implements HttpClientInterface, LoggerAwareInterface, ResetInterface
{
use HttpClientTrait;
use LoggerAwareTrait;
Expand Down Expand Up @@ -324,9 +325,17 @@ public function stream($responses, float $timeout = null): ResponseStreamInterfa
return new ResponseStream(CurlResponse::stream($responses, $timeout));
}

public function __destruct()
public function reset()
{
if ($this->logger) {
foreach ($this->multi->pushedResponses as $url => $response) {
$this->logger->debug(sprintf('Unused pushed response: "%s"', $url));
}
}

$this->multi->pushedResponses = [];
$this->multi->dnsCache->evictions = $this->multi->dnsCache->evictions ?: $this->multi->dnsCache->removals;
$this->multi->dnsCache->removals = $this->multi->dnsCache->hostnames = [];

if (\is_resource($this->multi->handle)) {
if (\defined('CURLMOPT_PUSHFUNCTION')) {
Expand All @@ -344,6 +353,11 @@ public function __destruct()
}
}

public function __destruct()
{
$this->reset();
}

private static function handlePush($parent, $pushed, array $requestHeaders, CurlClientState $multi, int $maxPendingPushes, ?LoggerInterface $logger): int
{
$headers = [];
Expand All @@ -363,12 +377,6 @@ private static function handlePush($parent, $pushed, array $requestHeaders, Curl

$url = $headers[':scheme'][0].'://'.$headers[':authority'][0];

if ($maxPendingPushes <= \count($multi->pushedResponses)) {
$logger && $logger->debug(sprintf('Rejecting pushed response from "%s" for "%s": the queue is full', $origin, $url));

return CURL_PUSH_DENY;
}

// curl before 7.65 doesn't validate the pushed ":authority" header,
// but this is a MUST in the HTTP/2 RFC; let's restrict pushes to the original host,
// ignoring domains mentioned as alt-name in the certificate for now (same as curl).
Expand All @@ -378,6 +386,12 @@ private static function handlePush($parent, $pushed, array $requestHeaders, Curl
return CURL_PUSH_DENY;
}

if ($maxPendingPushes <= \count($multi->pushedResponses)) {
$fifoUrl = key($multi->pushedResponses);
unset($multi->pushedResponses[$fifoUrl]);
$logger && $logger->debug(sprintf('Evicting oldest pushed response: "%s"', $fifoUrl));
}

$url .= $headers[':path'][0];
$logger && $logger->debug(sprintf('Queueing pushed response: "%s"', $url));

Expand Down
25 changes: 11 additions & 14 deletions src/Symfony/Component/HttpClient/DataCollector/HttpClientDataCollector.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,12 @@
use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\HttpFoundation\Response;
use Symfony\Component\HttpKernel\DataCollector\DataCollector;
use Symfony\Component\HttpKernel\DataCollector\LateDataCollectorInterface;

/**
* @author Jérémy Romey <jeremy@free-agent.fr>
*/
final class HttpClientDataCollector extends DataCollector
final class HttpClientDataCollector extends DataCollector implements LateDataCollectorInterface
{
/**
* @var TraceableHttpClient[]
Expand All @@ -38,7 +39,7 @@ public function registerClient(string $name, TraceableHttpClient $client)
*/
public function collect(Request $request, Response $response/*, \Throwable $exception = null*/)
{
$this->initData();
$this->reset();

foreach ($this->clients as $name => $client) {
[$errorCount, $traces] = $this->collectOnClient($client);
Expand All @@ -53,6 +54,13 @@ public function collect(Request $request, Response $response/*, \Throwable $exce
}
}

public function lateCollect()
{
foreach ($this->clients as $client) {
$client->reset();
}
}

public function getClients(): array
{
return $this->data['clients'] ?? [];
Expand All @@ -68,17 +76,6 @@ public function getErrorCount(): int
return $this->data['error_count'] ?? 0;
}

/**
* {@inheritdoc}
*/
public function reset()
{
$this->initData();
foreach ($this->clients as $client) {
$client->reset();
}
}

/**
* {@inheritdoc}
*/
Expand All @@ -87,7 +84,7 @@ public function getName(): string
return 'http_client';
}

private function initData()
public function reset()
{
$this->data = [
'clients' => [],
Expand Down
8 changes: 0 additions & 8 deletions src/Symfony/Component/HttpClient/Response/CurlResponse.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -228,15 +228,7 @@ public function __destruct()
} finally {
$this->close();

// Clear local caches when the only remaining handles are about pushed responses
if (!$this->multi->openHandles) {
if ($this->logger) {
foreach ($this->multi->pushedResponses as $url => $response) {
$this->logger->debug(sprintf('Unused pushed response: "%s"', $url));
}
}

$this->multi->pushedResponses = [];
// Schedule DNS cache eviction for the next request
$this->multi->dnsCache->evictions = $this->multi->dnsCache->evictions ?: $this->multi->dnsCache->removals;
$this->multi->dnsCache->removals = $this->multi->dnsCache->hostnames = [];
Expand Down
10 changes: 9 additions & 1 deletion src/Symfony/Component/HttpClient/ScopingHttpClient.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,13 +15,14 @@
use Symfony\Contracts\HttpClient\HttpClientInterface;
use Symfony\Contracts\HttpClient\ResponseInterface;
use Symfony\Contracts\HttpClient\ResponseStreamInterface;
use Symfony\Contracts\Service\ResetInterface;

/**
* Auto-configure the default options based on the requested URL.
*
* @author Anthony Martin <anthony.martin@sensiolabs.com>
*/
class ScopingHttpClient implements HttpClientInterface
class ScopingHttpClient implements HttpClientInterface, ResetInterface
{
use HttpClientTrait;

Expand Down Expand Up @@ -90,4 +91,11 @@ public function stream($responses, float $timeout = null): ResponseStreamInterfa
{
return $this->client->stream($responses, $timeout);
}

public function reset()
{
if ($this->client instanceof ResetInterface) {
$this->client->reset();
}
}
}
140 changes: 118 additions & 22 deletions src/Symfony/Component/HttpClient/Tests/CurlHttpClientTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,13 +13,23 @@

use Psr\Log\AbstractLogger;
use Symfony\Component\HttpClient\CurlHttpClient;
use Symfony\Component\Process\Exception\ProcessFailedException;
use Symfony\Component\Process\Process;
use Symfony\Contracts\HttpClient\HttpClientInterface;

/*
Tests for HTTP2 Push need a recent version of both PHP and curl. This docker command should run them:
docker run -it --rm -v $(pwd):/app -v /path/to/vulcain:/usr/local/bin/vulcain -w /app php:7.3-alpine ./phpunit src/Symfony/Component/HttpClient/Tests/CurlHttpClientTest.php --filter testHttp2Push
The vulcain binary can be found at https://github.com/symfony/binary-utils/releases/download/v0.1/vulcain_0.1.3_Linux_x86_64.tar.gz - see https://github.com/dunglas/vulcain for source
*/

/**
* @requires extension curl
*/
class CurlHttpClientTest extends HttpClientTestCase
{
private static $vulcainStarted = false;

protected function getHttpClient(string $testCase): HttpClientInterface
{
return new CurlHttpClient();
Expand All @@ -28,7 +38,81 @@ protected function getHttpClient(string $testCase): HttpClientInterface
/**
* @requires PHP 7.2.17
*/
public function testHttp2Push()
public function testHttp2PushVulcain()
{
$client = $this->getVulcainClient();
$logger = new TestLogger();
$client->setLogger($logger);

$responseAsArray = $client->request('GET', 'https://127.0.0.1:3000/json', [
'headers' => [
'Preload' => '/documents/*/id',
],
])->toArray();

foreach ($responseAsArray['documents'] as $document) {
$client->request('GET', 'https://127.0.0.1:3000'.$document['id'])->toArray();
}

$client->reset();

$expected = [
'Request: "GET https://127.0.0.1:3000/json"',
'Queueing pushed response: "https://127.0.0.1:3000/json/1"',
'Queueing pushed response: "https://127.0.0.1:3000/json/2"',
'Queueing pushed response: "https://127.0.0.1:3000/json/3"',
'Response: "200 https://127.0.0.1:3000/json"',
'Accepting pushed response: "GET https://127.0.0.1:3000/json/1"',
'Response: "200 https://127.0.0.1:3000/json/1"',
'Accepting pushed response: "GET https://127.0.0.1:3000/json/2"',
'Response: "200 https://127.0.0.1:3000/json/2"',
'Accepting pushed response: "GET https://127.0.0.1:3000/json/3"',
'Response: "200 https://127.0.0.1:3000/json/3"',
];
$this->assertSame($expected, $logger->logs);
}

/**
* @requires PHP 7.2.17
*/
public function testHttp2PushVulcainWithUnusedResponse()
{
$client = $this->getVulcainClient();
$logger = new TestLogger();
$client->setLogger($logger);

$responseAsArray = $client->request('GET', 'https://127.0.0.1:3000/json', [
'headers' => [
'Preload' => '/documents/*/id',
],
])->toArray();

$i = 0;
foreach ($responseAsArray['documents'] as $document) {
$client->request('GET', 'https://127.0.0.1:3000'.$document['id'])->toArray();
if (++$i >= 2) {
break;
}
}

$client->reset();

$expected = [
'Request: "GET https://127.0.0.1:3000/json"',
'Queueing pushed response: "https://127.0.0.1:3000/json/1"',
'Queueing pushed response: "https://127.0.0.1:3000/json/2"',
'Queueing pushed response: "https://127.0.0.1:3000/json/3"',
'Response: "200 https://127.0.0.1:3000/json"',
'Accepting pushed response: "GET https://127.0.0.1:3000/json/1"',
'Response: "200 https://127.0.0.1:3000/json/1"',
'Accepting pushed response: "GET https://127.0.0.1:3000/json/2"',
'Response: "200 https://127.0.0.1:3000/json/2"',
'Unused pushed response: "https://127.0.0.1:3000/json/3"',
];
$this->assertSame($expected, $logger->logs);
}

private function getVulcainClient(): CurlHttpClient
{
if (\PHP_VERSION_ID >= 70300 && \PHP_VERSION_ID < 70304) {
$this->markTestSkipped('PHP 7.3.0 to 7.3.3 don\'t support HTTP/2 PUSH');
Expand All @@ -38,32 +122,44 @@ public function testHttp2Push()
$this->markTestSkipped('curl <7.61 is used or it is not compiled with support for HTTP/2 PUSH');
}

$logger = new class() extends AbstractLogger {
public $logs = [];
$client = new CurlHttpClient(['verify_peer' => false, 'verify_host' => false]);

public function log($level, $message, array $context = []): void
{
$this->logs[] = $message;
}
};
if (static::$vulcainStarted) {
return $client;
}

$client = new CurlHttpClient([], 6, 2);
$client->setLogger($logger);
if (200 !== $client->request('GET', 'http://127.0.0.1:8057/json')->getStatusCode()) {
$this->markTestSkipped('symfony/http-client-contracts >= 2.0.1 required');
}

$index = $client->request('GET', 'https://http2.akamai.com/');
$index->getContent();
$process = new Process(['vulcain'], null, [
'DEBUG' => 1,
'UPSTREAM' => 'http://127.0.0.1:8057',
'ADDR' => ':3000',
'KEY_FILE' => __DIR__.'/Fixtures/tls/server.key',
'CERT_FILE' => __DIR__.'/Fixtures/tls/server.crt',
]);
$process->start();

$css = $client->request('GET', 'https://http2.akamai.com/resources/push.css');
register_shutdown_function([$process, 'stop']);
sleep('\\' === \DIRECTORY_SEPARATOR ? 10 : 1);

$css->getHeaders();
if (!$process->isRunning()) {
throw new ProcessFailedException($process);
}

$expected = [
'Request: "GET https://http2.akamai.com/"',
'Queueing pushed response: "https://http2.akamai.com/resources/push.css"',
'Response: "200 https://http2.akamai.com/"',
'Accepting pushed response: "GET https://http2.akamai.com/resources/push.css"',
'Response: "200 https://http2.akamai.com/resources/push.css"',
];
$this->assertSame($expected, $logger->logs);
static::$vulcainStarted = true;

return $client;
}
}

class TestLogger extends AbstractLogger
{
public $logs = [];

public function log($level, $message, array $context = []): void
{
$this->logs[] = $message;
}
}
Loading