Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit f48ebfa

Browse filesBrowse files
nicolas-grekasnicolas-grekas
committed
bug #33444 [HttpClient] improve handling of HTTP/2 PUSH, disable it by default (nicolas-grekas)
This PR was merged into the 4.3 branch. Discussion ---------- [HttpClient] improve handling of HTTP/2 PUSH, disable it by default | Q | A | ------------- | --- | Branch? | 4.3 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - This follows discussions with @dunglas For the test cases, https://http2-push.io is down, let's use Akamai instead This PR now considers the proxy settings before accepting a pushed response. It also splits the responsibility of dealing with accepting pushed responses in method `acceptPushForRequest`. The logic in this method could also be delegated to a userland callback passed as an option. Let's wait for someone with an actual use case before adding the option. This PR also disables HTTP/2 PUSH by default because it is not stable: locally, with the latest curl version, enabling this on a server that pushes things fails with `Failure when receiving data from the peer`. This is not ready for prime time in either ext-curl or the underlying libcurl. You can still enable it explicitly by passing some positive number to the constructor. Commits ------- 019bce7 [HttpClient] improve handling of HTTP/2 PUSH
2 parents 84fec70 + 019bce7 commit f48ebfa
Copy full SHA for f48ebfa

File tree

4 files changed

+55
-50
lines changed
Filter options

4 files changed

+55
-50
lines changed

‎src/Symfony/Component/HttpClient/CurlHttpClient.php

Copy file name to clipboardExpand all lines: src/Symfony/Component/HttpClient/CurlHttpClient.php
+39-28Lines changed: 39 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,7 @@ final class CurlHttpClient implements HttpClientInterface, LoggerAwareInterface
5555
*
5656
* @see HttpClientInterface::OPTIONS_DEFAULTS for available options
5757
*/
58-
public function __construct(array $defaultOptions = [], int $maxHostConnections = 6, int $maxPendingPushes = 50)
58+
public function __construct(array $defaultOptions = [], int $maxHostConnections = 6, int $maxPendingPushes = 0)
5959
{
6060
if (!\extension_loaded('curl')) {
6161
throw new \LogicException('You cannot use the "Symfony\Component\HttpClient\CurlHttpClient" as the "curl" extension is not installed.');
@@ -105,35 +105,30 @@ public function request(string $method, string $url, array $options = []): Respo
105105
$host = parse_url($authority, PHP_URL_HOST);
106106
$url = implode('', $url);
107107

108+
if (!isset($options['normalized_headers']['user-agent'])) {
109+
$options['normalized_headers']['user-agent'][] = 'Symfony HttpClient/Curl';
110+
$options['headers'][] = 'User-Agent: Symfony HttpClient/Curl';
111+
}
112+
108113
if ($pushedResponse = $this->multi->pushedResponses[$url] ?? null) {
109114
unset($this->multi->pushedResponses[$url]);
110-
// Accept pushed responses only if their headers related to authentication match the request
111-
$expectedHeaders = ['authorization', 'cookie', 'x-requested-with', 'range'];
112-
foreach ($expectedHeaders as $k => $v) {
113-
$expectedHeaders[$k] = null;
114-
115-
foreach ($options['normalized_headers'][$v] ?? [] as $h) {
116-
$expectedHeaders[$k][] = substr($h, 2 + \strlen($v));
117-
}
118-
}
119115

120-
if ('GET' === $method && $expectedHeaders === $pushedResponse->headers && !$options['body']) {
121-
$this->logger && $this->logger->debug(sprintf('Connecting request to pushed response: "%s %s"', $method, $url));
116+
if (self::acceptPushForRequest($method, $options, $pushedResponse)) {
117+
$this->logger && $this->logger->debug(sprintf('Accepting pushed response: "%s %s"', $method, $url));
122118

123119
// Reinitialize the pushed response with request's options
124120
$pushedResponse->response->__construct($this->multi, $url, $options, $this->logger);
125121

126122
return $pushedResponse->response;
127123
}
128124

129-
$this->logger && $this->logger->debug(sprintf('Rejecting pushed response for "%s": authorization headers don\'t match the request', $url));
125+
$this->logger && $this->logger->debug(sprintf('Rejecting pushed response: "%s".', $url));
130126
}
131127

132128
$this->logger && $this->logger->info(sprintf('Request: "%s %s"', $method, $url));
133129

134130
$curlopts = [
135131
CURLOPT_URL => $url,
136-
CURLOPT_USERAGENT => 'Symfony HttpClient/Curl',
137132
CURLOPT_TCP_NODELAY => true,
138133
CURLOPT_PROTOCOLS => CURLPROTO_HTTP | CURLPROTO_HTTPS,
139134
CURLOPT_REDIR_PROTOCOLS => CURLPROTO_HTTP | CURLPROTO_HTTPS,
@@ -306,7 +301,7 @@ public function __destruct()
306301
$active = 0;
307302
while (CURLM_CALL_MULTI_PERFORM === curl_multi_exec($this->multi->handle, $active));
308303

309-
foreach ($this->multi->openHandles as $ch) {
304+
foreach ($this->multi->openHandles as [$ch]) {
310305
curl_setopt($ch, CURLOPT_VERBOSE, false);
311306
}
312307
}
@@ -318,17 +313,17 @@ private static function handlePush($parent, $pushed, array $requestHeaders, Curl
318313

319314
foreach ($requestHeaders as $h) {
320315
if (false !== $i = strpos($h, ':', 1)) {
321-
$headers[substr($h, 0, $i)] = substr($h, 1 + $i);
316+
$headers[substr($h, 0, $i)][] = substr($h, 1 + $i);
322317
}
323318
}
324319

325-
if (!isset($headers[':method']) || !isset($headers[':scheme']) || !isset($headers[':authority']) || !isset($headers[':path']) || 'GET' !== $headers[':method'] || isset($headers['range'])) {
320+
if (!isset($headers[':method']) || !isset($headers[':scheme']) || !isset($headers[':authority']) || !isset($headers[':path'])) {
326321
$logger && $logger->debug(sprintf('Rejecting pushed response from "%s": pushed headers are invalid', $origin));
327322

328323
return CURL_PUSH_DENY;
329324
}
330325

331-
$url = $headers[':scheme'].'://'.$headers[':authority'];
326+
$url = $headers[':scheme'][0].'://'.$headers[':authority'][0];
332327

333328
if ($maxPendingPushes <= \count($multi->pushedResponses)) {
334329
$logger && $logger->debug(sprintf('Rejecting pushed response from "%s" for "%s": the queue is full', $origin, $url));
@@ -345,22 +340,38 @@ private static function handlePush($parent, $pushed, array $requestHeaders, Curl
345340
return CURL_PUSH_DENY;
346341
}
347342

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

351-
$multi->pushedResponses[$url] = new PushedResponse(
352-
new CurlResponse($multi, $pushed),
353-
[
354-
$headers['authorization'] ?? null,
355-
$headers['cookie'] ?? null,
356-
$headers['x-requested-with'] ?? null,
357-
null,
358-
]
359-
);
346+
$multi->pushedResponses[$url] = new PushedResponse(new CurlResponse($multi, $pushed), $headers, $multi->openHandles[(int) $parent][1] ?? []);
360347

361348
return CURL_PUSH_OK;
362349
}
363350

351+
/**
352+
* Accepts pushed responses only if their headers related to authentication match the request.
353+
*/
354+
private static function acceptPushForRequest(string $method, array $options, PushedResponse $pushedResponse): bool
355+
{
356+
if ($options['body'] || $method !== $pushedResponse->requestHeaders[':method'][0]) {
357+
return false;
358+
}
359+
360+
foreach (['proxy', 'no_proxy', 'bindto'] as $k) {
361+
if ($options[$k] !== $pushedResponse->parentOptions[$k]) {
362+
return false;
363+
}
364+
}
365+
366+
foreach (['authorization', 'cookie', 'range', 'proxy-authorization'] as $k) {
367+
if (($pushedResponse->requestHeaders[$k] ?? null) !== ($options['normalized_headers'][$k] ?? null)) {
368+
return false;
369+
}
370+
}
371+
372+
return true;
373+
}
374+
364375
/**
365376
* Wraps the request's body callback to allow it to return strings longer than curl requested.
366377
*/

‎src/Symfony/Component/HttpClient/Internal/PushedResponse.php

Copy file name to clipboardExpand all lines: src/Symfony/Component/HttpClient/Internal/PushedResponse.php
+7-5Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -14,23 +14,25 @@
1414
use Symfony\Component\HttpClient\Response\CurlResponse;
1515

1616
/**
17-
* A pushed response with headers.
17+
* A pushed response with its request headers.
1818
*
1919
* @author Alexander M. Turek <me@derrabus.de>
2020
*
2121
* @internal
2222
*/
2323
final class PushedResponse
2424
{
25-
/** @var CurlResponse */
2625
public $response;
2726

2827
/** @var string[] */
29-
public $headers;
28+
public $requestHeaders;
3029

31-
public function __construct(CurlResponse $response, array $headers)
30+
public $parentOptions = [];
31+
32+
public function __construct(CurlResponse $response, array $requestHeaders, array $parentOptions)
3233
{
3334
$this->response = $response;
34-
$this->headers = $headers;
35+
$this->requestHeaders = $requestHeaders;
36+
$this->parentOptions = $parentOptions;
3537
}
3638
}

‎src/Symfony/Component/HttpClient/Response/CurlResponse.php

Copy file name to clipboardExpand all lines: src/Symfony/Component/HttpClient/Response/CurlResponse.php
+1-4Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -120,9 +120,6 @@ public function __construct(CurlClientState $multi, $ch, array $options = null,
120120

121121
if (\in_array($waitFor, ['headers', 'destruct'], true)) {
122122
try {
123-
if (\defined('CURLOPT_STREAM_WEIGHT')) {
124-
curl_setopt($ch, CURLOPT_STREAM_WEIGHT, 32);
125-
}
126123
self::stream([$response])->current();
127124
} catch (\Throwable $e) {
128125
// Persist timeouts thrown during initialization
@@ -140,7 +137,7 @@ public function __construct(CurlClientState $multi, $ch, array $options = null,
140137
};
141138

142139
// Schedule the request in a non-blocking way
143-
$multi->openHandles[$id] = $ch;
140+
$multi->openHandles[$id] = [$ch, $options];
144141
curl_multi_add_handle($multi->handle, $ch);
145142
self::perform($multi);
146143
}

‎src/Symfony/Component/HttpClient/Tests/CurlHttpClientTest.php

Copy file name to clipboardExpand all lines: src/Symfony/Component/HttpClient/Tests/CurlHttpClientTest.php
+8-13Lines changed: 8 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -47,27 +47,22 @@ public function log($level, $message, array $context = [])
4747
}
4848
};
4949

50-
$client = new CurlHttpClient();
50+
$client = new CurlHttpClient([], 6, 2);
5151
$client->setLogger($logger);
5252

53-
$index = $client->request('GET', 'https://http2-push.io');
53+
$index = $client->request('GET', 'https://http2.akamai.com/');
5454
$index->getContent();
5555

56-
$css = $client->request('GET', 'https://http2-push.io/css/style.css');
57-
$js = $client->request('GET', 'https://http2-push.io/js/http2-push.js');
56+
$css = $client->request('GET', 'https://http2.akamai.com/resources/push.css');
5857

5958
$css->getHeaders();
60-
$js->getHeaders();
6159

6260
$expected = [
63-
'Request: "GET https://http2-push.io/"',
64-
'Queueing pushed response: "https://http2-push.io/css/style.css"',
65-
'Queueing pushed response: "https://http2-push.io/js/http2-push.js"',
66-
'Response: "200 https://http2-push.io/"',
67-
'Connecting request to pushed response: "GET https://http2-push.io/css/style.css"',
68-
'Connecting request to pushed response: "GET https://http2-push.io/js/http2-push.js"',
69-
'Response: "200 https://http2-push.io/css/style.css"',
70-
'Response: "200 https://http2-push.io/js/http2-push.js"',
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"',
7166
];
7267
$this->assertSame($expected, $logger->logs);
7368
}

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.