Skip to content

Commit 6eeca81

Browse files
julien-ncjulien-nc
committed
feat(IClient): let guzzle choose the handler for http requests so it can write a streamed response body progressively
Signed-off-by: Julien Veyssier <julien-nc@posteo.net>
1 parent fb67d5d commit 6eeca81

4 files changed

Lines changed: 85 additions & 51 deletions

File tree

‎lib/private/Http/Client/Client.php‎

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -41,15 +41,19 @@ public function __construct(
4141
}
4242

4343
private function buildRequestOptions(array $options): array {
44+
$streamResponse = !empty($options[RequestOptions::STREAM]);
4445
$proxy = $this->getProxyUri();
4546

4647
$defaults = [
4748
RequestOptions::VERIFY => $this->getCertBundle(),
4849
RequestOptions::TIMEOUT => IClient::DEFAULT_REQUEST_TIMEOUT,
49-
// Prefer HTTP/2 globally (PSR-7 request version)
50-
RequestOptions::VERSION => '2.0',
50+
// Guzzle's StreamHandler only supports HTTP/1.x, so streamed
51+
// responses must not force the default HTTP/2 transport settings.
52+
RequestOptions::VERSION => $streamResponse ? '1.1' : '2.0',
5153
];
52-
$defaults['curl'][\CURLOPT_HTTP_VERSION] = \CURL_HTTP_VERSION_2TLS;
54+
$defaults['curl'][\CURLOPT_HTTP_VERSION] = $streamResponse
55+
? \CURL_HTTP_VERSION_1_1
56+
: \CURL_HTTP_VERSION_2TLS;
5357

5458
$options['nextcloud']['allow_local_address'] = $this->isLocalAddressAllowed($options);
5559
if ($options['nextcloud']['allow_local_address'] === false) {
@@ -75,6 +79,12 @@ private function buildRequestOptions(array $options): array {
7579

7680
$options = array_merge($defaults, $options);
7781

82+
if ($streamResponse) {
83+
$options[RequestOptions::VERSION] = '1.1';
84+
$options['curl'] ??= [];
85+
$options['curl'][\CURLOPT_HTTP_VERSION] = \CURL_HTTP_VERSION_1_1;
86+
}
87+
7888
if (!isset($options[RequestOptions::HEADERS]['User-Agent'])) {
7989
$userAgent = 'Nextcloud-Server-Crawler/' . $this->serverVersion->getVersionString();
8090
$overwriteCliUrl = $this->config->getSystemValueString('overwrite.cli.url');

‎lib/private/Http/Client/ClientService.php‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,8 +9,8 @@
99
namespace OC\Http\Client;
1010

1111
use GuzzleHttp\Client as GuzzleClient;
12-
use GuzzleHttp\Handler\CurlHandler;
1312
use GuzzleHttp\HandlerStack;
13+
use GuzzleHttp\Utils;
1414
use GuzzleHttp\Middleware;
1515
use OCP\Diagnostics\IEventLogger;
1616
use OCP\Http\Client\IClient;
@@ -41,7 +41,7 @@ public function __construct(
4141

4242
#[\Override]
4343
public function newClient(): IClient {
44-
$handler = new CurlHandler();
44+
$handler = Utils::chooseHandler();
4545
$stack = HandlerStack::create($handler);
4646
if ($this->config->getSystemValueBool('dns_pinning', true)) {
4747
$stack->push($this->dnsPinMiddleware->addDnsPinning());

‎tests/lib/Http/Client/ClientServiceTest.php‎

Lines changed: 2 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -10,10 +10,6 @@
1010

1111
namespace Test\Http\Client;
1212

13-
use GuzzleHttp\Client as GuzzleClient;
14-
use GuzzleHttp\Handler\CurlHandler;
15-
use GuzzleHttp\HandlerStack;
16-
use GuzzleHttp\Middleware;
1713
use OC\Http\Client\Client;
1814
use OC\Http\Client\ClientService;
1915
use OC\Http\Client\DnsPinMiddleware;
@@ -22,7 +18,6 @@
2218
use OCP\IConfig;
2319
use OCP\Security\IRemoteHostValidator;
2420
use OCP\ServerVersion;
25-
use Psr\Http\Message\RequestInterface;
2621
use Psr\Log\LoggerInterface;
2722

2823
/**
@@ -58,27 +53,7 @@ public function testNewClient(): void {
5853
$serverVersion,
5954
);
6055

61-
$handler = new CurlHandler();
62-
$stack = HandlerStack::create($handler);
63-
$stack->push($dnsPinMiddleware->addDnsPinning());
64-
$stack->push(Middleware::tap(function (RequestInterface $request) use ($eventLogger): void {
65-
$eventLogger->start('http:request', $request->getMethod() . ' request to ' . $request->getRequestTarget());
66-
}, function () use ($eventLogger): void {
67-
$eventLogger->end('http:request');
68-
}), 'event logger');
69-
$guzzleClient = new GuzzleClient(['handler' => $stack]);
70-
71-
$this->assertEquals(
72-
new Client(
73-
$config,
74-
$certificateManager,
75-
$guzzleClient,
76-
$remoteHostValidator,
77-
$logger,
78-
$serverVersion,
79-
),
80-
$clientService->newClient()
81-
);
56+
$this->assertInstanceOf(Client::class, $clientService->newClient());
8257
}
8358

8459
public function testDisableDnsPinning(): void {
@@ -110,25 +85,6 @@ public function testDisableDnsPinning(): void {
11085
$serverVersion,
11186
);
11287

113-
$handler = new CurlHandler();
114-
$stack = HandlerStack::create($handler);
115-
$stack->push(Middleware::tap(function (RequestInterface $request) use ($eventLogger): void {
116-
$eventLogger->start('http:request', $request->getMethod() . ' request to ' . $request->getRequestTarget());
117-
}, function () use ($eventLogger): void {
118-
$eventLogger->end('http:request');
119-
}), 'event logger');
120-
$guzzleClient = new GuzzleClient(['handler' => $stack]);
121-
122-
$this->assertEquals(
123-
new Client(
124-
$config,
125-
$certificateManager,
126-
$guzzleClient,
127-
$remoteHostValidator,
128-
$logger,
129-
$serverVersion,
130-
),
131-
$clientService->newClient()
132-
);
88+
$this->assertInstanceOf(Client::class, $clientService->newClient());
13389
}
13490
}

‎tests/lib/Http/Client/ClientTest.php‎

Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -321,6 +321,23 @@ public function testGet(): void {
321321
$this->assertEquals(418, $this->client->get('http://localhost/', [])->getStatusCode());
322322
}
323323

324+
public function testGetStreamUsesHttp11(): void {
325+
$this->setUpDefaultRequestOptions();
326+
327+
$options = array_merge($this->defaultRequestOptions, [
328+
'stream' => true,
329+
'version' => '1.1',
330+
'curl' => [
331+
\CURLOPT_HTTP_VERSION => \CURL_HTTP_VERSION_1_1,
332+
],
333+
]);
334+
335+
$this->guzzleClient->method('request')
336+
->with('get', 'http://localhost/', $options)
337+
->willReturn(new Response(418));
338+
$this->assertEquals(418, $this->client->get('http://localhost/', ['stream' => true])->getStatusCode());
339+
}
340+
324341
public function testGetWithOptions(): void {
325342
$this->setUpDefaultRequestOptions();
326343

@@ -522,6 +539,57 @@ public function testSetDefaultOptionsWithNotInstalled(): void {
522539
], self::invokePrivate($this->client, 'buildRequestOptions', [[]]));
523540
}
524541

542+
public function testSetDefaultOptionsWithStream(): void {
543+
$this->config
544+
->expects($this->exactly(3))
545+
->method('getSystemValueBool')
546+
->willReturnMap([
547+
['installed', false, true],
548+
['allow_local_remote_servers', false, false],
549+
['http_client_add_user_agent_url', false, false],
550+
]);
551+
$this->config
552+
->expects($this->exactly(2))
553+
->method('getSystemValueString')
554+
->willReturnMap([
555+
['proxy', '', ''],
556+
['overwrite.cli.url', '', ''],
557+
]);
558+
$this->certificateManager
559+
->expects($this->once())
560+
->method('getAbsoluteBundlePath')
561+
->with()
562+
->willReturn('/my/path.crt');
563+
564+
$this->serverVersion->method('getVersionString')
565+
->willReturn('123.45.6');
566+
567+
$this->assertEquals([
568+
'verify' => '/my/path.crt',
569+
'headers' => [
570+
'User-Agent' => 'Nextcloud-Server-Crawler/123.45.6',
571+
'Accept-Encoding' => 'gzip',
572+
],
573+
'timeout' => 30,
574+
'nextcloud' => [
575+
'allow_local_address' => false,
576+
],
577+
'allow_redirects' => [
578+
'on_redirect' => function (
579+
\Psr\Http\Message\RequestInterface $request,
580+
\Psr\Http\Message\ResponseInterface $response,
581+
\Psr\Http\Message\UriInterface $uri,
582+
): void {
583+
},
584+
],
585+
'stream' => true,
586+
'version' => '1.1',
587+
'curl' => [
588+
\CURLOPT_HTTP_VERSION => \CURL_HTTP_VERSION_1_1,
589+
],
590+
], self::invokePrivate($this->client, 'buildRequestOptions', [['stream' => true]]));
591+
}
592+
525593
public function testSetDefaultOptionsWithProxy(): void {
526594
$this->config
527595
->expects($this->exactly(3))

0 commit comments

Comments
 (0)