Skip to content

Commit 5da54cc

Browse files
lyrixxnicolas-grekas
lyrixx
authored and
nicolas-grekas
committed
[HttpClient] Fix early cleanup of pushed HTTP/2 responses
1 parent 233f73b commit 5da54cc

File tree

9 files changed

+216
-57
lines changed

9 files changed

+216
-57
lines changed

‎CurlHttpClient.php

Lines changed: 22 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@
2323
use Symfony\Contracts\HttpClient\HttpClientInterface;
2424
use Symfony\Contracts\HttpClient\ResponseInterface;
2525
use Symfony\Contracts\HttpClient\ResponseStreamInterface;
26+
use Symfony\Contracts\Service\ResetInterface;
2627

2728
/**
2829
* A performant implementation of the HttpClientInterface contracts based on the curl extension.
@@ -32,7 +33,7 @@
3233
*
3334
* @author Nicolas Grekas <[email protected]>
3435
*/
35-
final class CurlHttpClient implements HttpClientInterface, LoggerAwareInterface
36+
final class CurlHttpClient implements HttpClientInterface, LoggerAwareInterface, ResetInterface
3637
{
3738
use HttpClientTrait;
3839
use LoggerAwareTrait;
@@ -324,9 +325,17 @@ public function stream($responses, float $timeout = null): ResponseStreamInterfa
324325
return new ResponseStream(CurlResponse::stream($responses, $timeout));
325326
}
326327

327-
public function __destruct()
328+
public function reset()
328329
{
330+
if ($this->logger) {
331+
foreach ($this->multi->pushedResponses as $url => $response) {
332+
$this->logger->debug(sprintf('Unused pushed response: "%s"', $url));
333+
}
334+
}
335+
329336
$this->multi->pushedResponses = [];
337+
$this->multi->dnsCache->evictions = $this->multi->dnsCache->evictions ?: $this->multi->dnsCache->removals;
338+
$this->multi->dnsCache->removals = $this->multi->dnsCache->hostnames = [];
330339

331340
if (\is_resource($this->multi->handle)) {
332341
if (\defined('CURLMOPT_PUSHFUNCTION')) {
@@ -344,6 +353,11 @@ public function __destruct()
344353
}
345354
}
346355

356+
public function __destruct()
357+
{
358+
$this->reset();
359+
}
360+
347361
private static function handlePush($parent, $pushed, array $requestHeaders, CurlClientState $multi, int $maxPendingPushes, ?LoggerInterface $logger): int
348362
{
349363
$headers = [];
@@ -363,12 +377,6 @@ private static function handlePush($parent, $pushed, array $requestHeaders, Curl
363377

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

366-
if ($maxPendingPushes <= \count($multi->pushedResponses)) {
367-
$logger && $logger->debug(sprintf('Rejecting pushed response from "%s" for "%s": the queue is full', $origin, $url));
368-
369-
return CURL_PUSH_DENY;
370-
}
371-
372380
// curl before 7.65 doesn't validate the pushed ":authority" header,
373381
// but this is a MUST in the HTTP/2 RFC; let's restrict pushes to the original host,
374382
// ignoring domains mentioned as alt-name in the certificate for now (same as curl).
@@ -378,6 +386,12 @@ private static function handlePush($parent, $pushed, array $requestHeaders, Curl
378386
return CURL_PUSH_DENY;
379387
}
380388

389+
if ($maxPendingPushes <= \count($multi->pushedResponses)) {
390+
$fifoUrl = key($multi->pushedResponses);
391+
unset($multi->pushedResponses[$fifoUrl]);
392+
$logger && $logger->debug(sprintf('Evicting oldest pushed response: "%s"', $fifoUrl));
393+
}
394+
381395
$url .= $headers[':path'][0];
382396
$logger && $logger->debug(sprintf('Queueing pushed response: "%s"', $url));
383397

‎DataCollector/HttpClientDataCollector.php

Lines changed: 11 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -15,11 +15,12 @@
1515
use Symfony\Component\HttpFoundation\Request;
1616
use Symfony\Component\HttpFoundation\Response;
1717
use Symfony\Component\HttpKernel\DataCollector\DataCollector;
18+
use Symfony\Component\HttpKernel\DataCollector\LateDataCollectorInterface;
1819

1920
/**
2021
* @author Jérémy Romey <[email protected]>
2122
*/
22-
final class HttpClientDataCollector extends DataCollector
23+
final class HttpClientDataCollector extends DataCollector implements LateDataCollectorInterface
2324
{
2425
/**
2526
* @var TraceableHttpClient[]
@@ -38,7 +39,7 @@ public function registerClient(string $name, TraceableHttpClient $client)
3839
*/
3940
public function collect(Request $request, Response $response/*, \Throwable $exception = null*/)
4041
{
41-
$this->initData();
42+
$this->reset();
4243

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

57+
public function lateCollect()
58+
{
59+
foreach ($this->clients as $client) {
60+
$client->reset();
61+
}
62+
}
63+
5664
public function getClients(): array
5765
{
5866
return $this->data['clients'] ?? [];
@@ -68,17 +76,6 @@ public function getErrorCount(): int
6876
return $this->data['error_count'] ?? 0;
6977
}
7078

71-
/**
72-
* {@inheritdoc}
73-
*/
74-
public function reset()
75-
{
76-
$this->initData();
77-
foreach ($this->clients as $client) {
78-
$client->reset();
79-
}
80-
}
81-
8279
/**
8380
* {@inheritdoc}
8481
*/
@@ -87,7 +84,7 @@ public function getName(): string
8784
return 'http_client';
8885
}
8986

90-
private function initData()
87+
public function reset()
9188
{
9289
$this->data = [
9390
'clients' => [],

‎Response/CurlResponse.php

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -228,15 +228,7 @@ public function __destruct()
228228
} finally {
229229
$this->close();
230230

231-
// Clear local caches when the only remaining handles are about pushed responses
232231
if (!$this->multi->openHandles) {
233-
if ($this->logger) {
234-
foreach ($this->multi->pushedResponses as $url => $response) {
235-
$this->logger->debug(sprintf('Unused pushed response: "%s"', $url));
236-
}
237-
}
238-
239-
$this->multi->pushedResponses = [];
240232
// Schedule DNS cache eviction for the next request
241233
$this->multi->dnsCache->evictions = $this->multi->dnsCache->evictions ?: $this->multi->dnsCache->removals;
242234
$this->multi->dnsCache->removals = $this->multi->dnsCache->hostnames = [];

‎ScopingHttpClient.php

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,13 +15,14 @@
1515
use Symfony\Contracts\HttpClient\HttpClientInterface;
1616
use Symfony\Contracts\HttpClient\ResponseInterface;
1717
use Symfony\Contracts\HttpClient\ResponseStreamInterface;
18+
use Symfony\Contracts\Service\ResetInterface;
1819

1920
/**
2021
* Auto-configure the default options based on the requested URL.
2122
*
2223
* @author Anthony Martin <[email protected]>
2324
*/
24-
class ScopingHttpClient implements HttpClientInterface
25+
class ScopingHttpClient implements HttpClientInterface, ResetInterface
2526
{
2627
use HttpClientTrait;
2728

@@ -90,4 +91,11 @@ public function stream($responses, float $timeout = null): ResponseStreamInterfa
9091
{
9192
return $this->client->stream($responses, $timeout);
9293
}
94+
95+
public function reset()
96+
{
97+
if ($this->client instanceof ResetInterface) {
98+
$this->client->reset();
99+
}
100+
}
93101
}

‎Tests/CurlHttpClientTest.php

Lines changed: 118 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -13,13 +13,23 @@
1313

1414
use Psr\Log\AbstractLogger;
1515
use Symfony\Component\HttpClient\CurlHttpClient;
16+
use Symfony\Component\Process\Exception\ProcessFailedException;
17+
use Symfony\Component\Process\Process;
1618
use Symfony\Contracts\HttpClient\HttpClientInterface;
1719

20+
/*
21+
Tests for HTTP2 Push need a recent version of both PHP and curl. This docker command should run them:
22+
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
23+
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
24+
*/
25+
1826
/**
1927
* @requires extension curl
2028
*/
2129
class CurlHttpClientTest extends HttpClientTestCase
2230
{
31+
private static $vulcainStarted = false;
32+
2333
protected function getHttpClient(string $testCase): HttpClientInterface
2434
{
2535
return new CurlHttpClient();
@@ -28,7 +38,81 @@ protected function getHttpClient(string $testCase): HttpClientInterface
2838
/**
2939
* @requires PHP 7.2.17
3040
*/
31-
public function testHttp2Push()
41+
public function testHttp2PushVulcain()
42+
{
43+
$client = $this->getVulcainClient();
44+
$logger = new TestLogger();
45+
$client->setLogger($logger);
46+
47+
$responseAsArray = $client->request('GET', 'https://127.0.0.1:3000/json', [
48+
'headers' => [
49+
'Preload' => '/documents/*/id',
50+
],
51+
])->toArray();
52+
53+
foreach ($responseAsArray['documents'] as $document) {
54+
$client->request('GET', 'https://127.0.0.1:3000'.$document['id'])->toArray();
55+
}
56+
57+
$client->reset();
58+
59+
$expected = [
60+
'Request: "GET https://127.0.0.1:3000/json"',
61+
'Queueing pushed response: "https://127.0.0.1:3000/json/1"',
62+
'Queueing pushed response: "https://127.0.0.1:3000/json/2"',
63+
'Queueing pushed response: "https://127.0.0.1:3000/json/3"',
64+
'Response: "200 https://127.0.0.1:3000/json"',
65+
'Accepting pushed response: "GET https://127.0.0.1:3000/json/1"',
66+
'Response: "200 https://127.0.0.1:3000/json/1"',
67+
'Accepting pushed response: "GET https://127.0.0.1:3000/json/2"',
68+
'Response: "200 https://127.0.0.1:3000/json/2"',
69+
'Accepting pushed response: "GET https://127.0.0.1:3000/json/3"',
70+
'Response: "200 https://127.0.0.1:3000/json/3"',
71+
];
72+
$this->assertSame($expected, $logger->logs);
73+
}
74+
75+
/**
76+
* @requires PHP 7.2.17
77+
*/
78+
public function testHttp2PushVulcainWithUnusedResponse()
79+
{
80+
$client = $this->getVulcainClient();
81+
$logger = new TestLogger();
82+
$client->setLogger($logger);
83+
84+
$responseAsArray = $client->request('GET', 'https://127.0.0.1:3000/json', [
85+
'headers' => [
86+
'Preload' => '/documents/*/id',
87+
],
88+
])->toArray();
89+
90+
$i = 0;
91+
foreach ($responseAsArray['documents'] as $document) {
92+
$client->request('GET', 'https://127.0.0.1:3000'.$document['id'])->toArray();
93+
if (++$i >= 2) {
94+
break;
95+
}
96+
}
97+
98+
$client->reset();
99+
100+
$expected = [
101+
'Request: "GET https://127.0.0.1:3000/json"',
102+
'Queueing pushed response: "https://127.0.0.1:3000/json/1"',
103+
'Queueing pushed response: "https://127.0.0.1:3000/json/2"',
104+
'Queueing pushed response: "https://127.0.0.1:3000/json/3"',
105+
'Response: "200 https://127.0.0.1:3000/json"',
106+
'Accepting pushed response: "GET https://127.0.0.1:3000/json/1"',
107+
'Response: "200 https://127.0.0.1:3000/json/1"',
108+
'Accepting pushed response: "GET https://127.0.0.1:3000/json/2"',
109+
'Response: "200 https://127.0.0.1:3000/json/2"',
110+
'Unused pushed response: "https://127.0.0.1:3000/json/3"',
111+
];
112+
$this->assertSame($expected, $logger->logs);
113+
}
114+
115+
private function getVulcainClient(): CurlHttpClient
32116
{
33117
if (\PHP_VERSION_ID >= 70300 && \PHP_VERSION_ID < 70304) {
34118
$this->markTestSkipped('PHP 7.3.0 to 7.3.3 don\'t support HTTP/2 PUSH');
@@ -38,32 +122,44 @@ public function testHttp2Push()
38122
$this->markTestSkipped('curl <7.61 is used or it is not compiled with support for HTTP/2 PUSH');
39123
}
40124

41-
$logger = new class() extends AbstractLogger {
42-
public $logs = [];
125+
$client = new CurlHttpClient(['verify_peer' => false, 'verify_host' => false]);
43126

44-
public function log($level, $message, array $context = []): void
45-
{
46-
$this->logs[] = $message;
47-
}
48-
};
127+
if (static::$vulcainStarted) {
128+
return $client;
129+
}
49130

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

53-
$index = $client->request('GET', 'https://http2.akamai.com/');
54-
$index->getContent();
135+
$process = new Process(['vulcain'], null, [
136+
'DEBUG' => 1,
137+
'UPSTREAM' => 'http://127.0.0.1:8057',
138+
'ADDR' => ':3000',
139+
'KEY_FILE' => __DIR__.'/Fixtures/tls/server.key',
140+
'CERT_FILE' => __DIR__.'/Fixtures/tls/server.crt',
141+
]);
142+
$process->start();
55143

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

58-
$css->getHeaders();
147+
if (!$process->isRunning()) {
148+
throw new ProcessFailedException($process);
149+
}
59150

60-
$expected = [
61-
'Request: "GET https://http2.akamai.com/"',
62-
'Queueing pushed response: "https://http2.akamai.com/resources/push.css"',
63-
'Response: "200 https://http2.akamai.com/"',
64-
'Accepting pushed response: "GET https://http2.akamai.com/resources/push.css"',
65-
'Response: "200 https://http2.akamai.com/resources/push.css"',
66-
];
67-
$this->assertSame($expected, $logger->logs);
151+
static::$vulcainStarted = true;
152+
153+
return $client;
154+
}
155+
}
156+
157+
class TestLogger extends AbstractLogger
158+
{
159+
public $logs = [];
160+
161+
public function log($level, $message, array $context = []): void
162+
{
163+
$this->logs[] = $message;
68164
}
69165
}

‎Tests/Fixtures/tls/server.crt

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
-----BEGIN CERTIFICATE-----
2+
MIIDPjCCAiYCCQDpVvfmCZt2GzANBgkqhkiG9w0BAQsFADBhMQswCQYDVQQGEwJV
3+
UzEUMBIGA1UEBwwLR290aGFtIENpdHkxEjAQBgNVBAMMCWxvY2FsaG9zdDEoMCYG
4+
CSqGSIb3DQEJARYZZHVuZ2xhcyttZXJjdXJlQGdtYWlsLmNvbTAeFw0xOTAxMjMx
5+
NTUzMzlaFw0yOTAxMjAxNTUzMzlaMGExCzAJBgNVBAYTAlVTMRQwEgYDVQQHDAtH
6+
b3RoYW0gQ2l0eTESMBAGA1UEAwwJbG9jYWxob3N0MSgwJgYJKoZIhvcNAQkBFhlk
7+
dW5nbGFzK21lcmN1cmVAZ21haWwuY29tMIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8A
8+
MIIBCgKCAQEAuKnXkBSJwOwkKfR58wP/yLYW9QFX2THoqN8iffangRmZwc5KLE6F
9+
1S8jYMv3JGiJ95Ij3MezAfuBCdgPqqP8JrR1XwjR1RFZMOL/4U9R9OuMVng04PLw
10+
L6TzKoEtZuExHUWFP0+5AYblgno2hoN/HVuox8m6zQrBNcbhTgDIjP5Hn491d9od
11+
MtS3OxksDLr1UIOUGUWF7MQMN7lsN7rgT5qxoCkcAGAB4GPOA23HMt2zt4afDiI7
12+
lAmuv8MKkTmBCcFe+q+U7o6wMxkjGstzAWRibtwzR4ejPwdO7se23MXCWGPvF16Z
13+
tu1ip+e+waRus9o5UnyGaVPFAw8iCTC/KwIDAQABMA0GCSqGSIb3DQEBCwUAA4IB
14+
AQB42AW7E57yOky8GpsKLoa9u7okwvvg8CQJ117X8a2MElBGnmMd9tjLa/pXAx2I
15+
bN7jSTSadXiPNYCx4ueiJa4Dwy+C8YkwUbhRf3+mc7Chnz0SXouTjh7OUeeA06jS
16+
W2VAR2pKB0pdJtAkXxIy21Juu8KF5uZqVq1oimgKw2lRUIMdKaqsrVwESk6u5Ojj
17+
3DS40q9DzFnwKGCuZpspvMdWYLscotzLrCbnHp+guWDigEHS3CKzKbNo327nVg6X
18+
7UjqqtPZ2mCsnUx3QTDJsr3gcSqhzmB+Q6I/0Q2Nx/aMmbsNegu+LC3GjFtL59Bv
19+
B8pB/MxID0j47SwPKQghZvb3
20+
-----END CERTIFICATE-----

0 commit comments

Comments
 (0)