Skip to content

Commit 8d25d58

Browse files
nicolas-grekasnicolas-grekas
committed
bug #34554 [HttpClient] Fix early cleanup of pushed HTTP/2 responses (lyrixx)
This PR was merged into the 4.4 branch. Discussion ---------- [HttpClient] Fix early cleanup of pushed HTTP/2 responses | Q | A | ------------- | --- | Branch? | 4.4 | Bug fix? | no | New feature? | no | Deprecations? | no | Tickets | | License | MIT | Doc PR | Commits ------- 0f51da6ec7 [HttpClient] Fix early cleanup of pushed HTTP/2 responses
1 parent 955c977 commit 8d25d58

File tree

7 files changed

+198
-40
lines changed

7 files changed

+198
-40
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.
@@ -34,7 +35,7 @@
3435
*
3536
* @experimental in 4.3
3637
*/
37-
final class CurlHttpClient implements HttpClientInterface, LoggerAwareInterface
38+
final class CurlHttpClient implements HttpClientInterface, LoggerAwareInterface, ResetInterface
3839
{
3940
use HttpClientTrait;
4041
use LoggerAwareTrait;
@@ -298,9 +299,17 @@ public function stream($responses, float $timeout = null): ResponseStreamInterfa
298299
return new ResponseStream(CurlResponse::stream($responses, $timeout));
299300
}
300301

301-
public function __destruct()
302+
public function reset()
302303
{
304+
if ($this->logger) {
305+
foreach ($this->multi->pushedResponses as $url => $response) {
306+
$this->logger->debug(sprintf('Unused pushed response: "%s"', $url));
307+
}
308+
}
309+
303310
$this->multi->pushedResponses = [];
311+
$this->multi->dnsCache->evictions = $this->multi->dnsCache->evictions ?: $this->multi->dnsCache->removals;
312+
$this->multi->dnsCache->removals = $this->multi->dnsCache->hostnames = [];
304313

305314
if (\is_resource($this->multi->handle)) {
306315
if (\defined('CURLMOPT_PUSHFUNCTION')) {
@@ -318,6 +327,11 @@ public function __destruct()
318327
}
319328
}
320329

330+
public function __destruct()
331+
{
332+
$this->reset();
333+
}
334+
321335
private static function handlePush($parent, $pushed, array $requestHeaders, CurlClientState $multi, int $maxPendingPushes, ?LoggerInterface $logger): int
322336
{
323337
$headers = [];
@@ -337,12 +351,6 @@ private static function handlePush($parent, $pushed, array $requestHeaders, Curl
337351

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

340-
if ($maxPendingPushes <= \count($multi->pushedResponses)) {
341-
$logger && $logger->debug(sprintf('Rejecting pushed response from "%s" for "%s": the queue is full', $origin, $url));
342-
343-
return CURL_PUSH_DENY;
344-
}
345-
346354
// curl before 7.65 doesn't validate the pushed ":authority" header,
347355
// but this is a MUST in the HTTP/2 RFC; let's restrict pushes to the original host,
348356
// ignoring domains mentioned as alt-name in the certificate for now (same as curl).
@@ -352,6 +360,12 @@ private static function handlePush($parent, $pushed, array $requestHeaders, Curl
352360
return CURL_PUSH_DENY;
353361
}
354362

363+
if ($maxPendingPushes <= \count($multi->pushedResponses)) {
364+
$fifoUrl = key($multi->pushedResponses);
365+
unset($multi->pushedResponses[$fifoUrl]);
366+
$logger && $logger->debug(sprintf('Evicting oldest pushed response: "%s"', $fifoUrl));
367+
}
368+
355369
$url .= $headers[':path'][0];
356370
$logger && $logger->debug(sprintf('Queueing pushed response: "%s"', $url));
357371

‎Response/CurlResponse.php

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

211-
// Clear local caches when the only remaining handles are about pushed responses
212211
if (!$this->multi->openHandles) {
213-
if ($this->logger) {
214-
foreach ($this->multi->pushedResponses as $url => $response) {
215-
$this->logger->debug(sprintf('Unused pushed response: "%s"', $url));
216-
}
217-
}
218-
219-
$this->multi->pushedResponses = [];
220212
// Schedule DNS cache eviction for the next request
221213
$this->multi->dnsCache->evictions = $this->multi->dnsCache->evictions ?: $this->multi->dnsCache->removals;
222214
$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,6 +15,7 @@
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.
@@ -23,7 +24,7 @@
2324
*
2425
* @experimental in 4.3
2526
*/
26-
class ScopingHttpClient implements HttpClientInterface
27+
class ScopingHttpClient implements HttpClientInterface, ResetInterface
2728
{
2829
use HttpClientTrait;
2930

@@ -92,4 +93,11 @@ public function stream($responses, float $timeout = null): ResponseStreamInterfa
9293
{
9394
return $this->client->stream($responses, $timeout);
9495
}
96+
97+
public function reset()
98+
{
99+
if ($this->client instanceof ResetInterface) {
100+
$this->client->reset();
101+
}
102+
}
95103
}

‎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 = [])
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 (['application/json'] !== $client->request('GET', 'http://127.0.0.1:8057/json')->getHeaders()['content-type']) {
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-----

‎Tests/Fixtures/tls/server.key

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
-----BEGIN RSA PRIVATE KEY-----
2+
MIIEpAIBAAKCAQEAuKnXkBSJwOwkKfR58wP/yLYW9QFX2THoqN8iffangRmZwc5K
3+
LE6F1S8jYMv3JGiJ95Ij3MezAfuBCdgPqqP8JrR1XwjR1RFZMOL/4U9R9OuMVng0
4+
4PLwL6TzKoEtZuExHUWFP0+5AYblgno2hoN/HVuox8m6zQrBNcbhTgDIjP5Hn491
5+
d9odMtS3OxksDLr1UIOUGUWF7MQMN7lsN7rgT5qxoCkcAGAB4GPOA23HMt2zt4af
6+
DiI7lAmuv8MKkTmBCcFe+q+U7o6wMxkjGstzAWRibtwzR4ejPwdO7se23MXCWGPv
7+
F16Ztu1ip+e+waRus9o5UnyGaVPFAw8iCTC/KwIDAQABAoIBAQCczVNGe7oRADMh
8+
EP/wM4ghhUTvHAndWrzFkFs4fJX1UKi34ZQoFTEdOZ6f1fHwj3f/qa8cDNJar5X9
9+
puJ+siotL3Suks2iT83dbhN63SCpiM2sqvuzu3Xp7vWwNOo5fqR2x46CmQ5uVn5S
10+
EbZ09/mbEza5FvmwnB49rLepxY6F8P+vK5ZnCZYS2SHpOxv3U9wG8gmcHRI9ejbC
11+
X9rwuu3oT23bfbJ0tn6Qh8O3R1kXZUUXqnxsn554cZZrXg5+ygbt4HfDVWMLpqy/
12+
5wG0FCpU8QvjF4L8qErP7TZRrWGFtti1RtACbu9LrWvO/74v54td5V28U6kqlDJR
13+
ff4Mi4whAoGBAOBzReQIxGwoYApPyhF+ohvF39JEEXYfkzk94t6hbgyBFBFvqdFY
14+
shT59im2P9LyDvTd5DnCIo52Sj7vM9H80tRjAA0A8okGOczk31ABbH8aZ2orU/0G
15+
EJe4PV4r3bpLO6DKTYsicgRpXI3aHHLvYFXOVNrQKfrKCQ+GFMVuhDdRAoGBANKe
16+
3Dn3XOq7EW42GZey1xUxrfQRJp491KXHvjYt7z7zSiUzqN+mqIqz6ngCjJWbyQsl
17+
Ud9N9U+4rNfYYLHQ0resjxGQRtmooOHlLhT6pEplXDgQb2SmCg2u22SKkkXA7zOV
18+
OFbNryXgkYThsA6ih8LiKM8aFn7zttRSEeTpfye7AoGBALhIzRyiuiuXpuswgdeF
19+
YrJs8A1jB/c1i5qXHlvurT2lCYYbaZHSQj0I0r2CvrqDNhaEzStDIz5XDzTHD4Qd
20+
EjmBo3wJyBkLPI/nZxb4ZE2jrz8znf0EasE3a2OTnrSjqqylDa/sMzM+EtkBORSB
21+
SFaLV45lFeKs2W2eiBVmXTZRAoGAJoA7qaz6Iz6G9SqWixB6GLm4HsFz2cFbueJF
22+
dwn2jf9TMnG7EQcaECDLX5y3rjGIEq2DxdouWaBcmChJpLeTjVfR31gMW4Vjw2dt
23+
gRBAMAlPTkBS3Ictl0q7eCmMi4u1Liy828FFnxrp/uxyjnpPbuSAqTsPma1bYnyO
24+
INY+FDkCgYAe9e39/vXe7Un3ysjqDUW+0OMM+kg4ulhiopzKY+QbHiSWmUUDtvcN
25+
asqrYiX1d59e2ZNiqrlBn86I8549St81bWSrRMNf7R+WVb79RApsABeUaEoyo3lq
26+
0UgOBM8Nt558kaja/YfJf/jwNC1DPuu5x5t38ZcqAkqrZ/HEPkFdGQ==
27+
-----END RSA PRIVATE KEY-----

‎composer.json

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,8 @@
2222
"php": "^7.1.3",
2323
"psr/log": "^1.0",
2424
"symfony/http-client-contracts": "^1.1.7",
25-
"symfony/polyfill-php73": "^1.11"
25+
"symfony/polyfill-php73": "^1.11",
26+
"symfony/service-contracts": "^1.0|^2"
2627
},
2728
"require-dev": {
2829
"nyholm/psr7": "^1.0",

0 commit comments

Comments
 (0)