8000 bug #38530 [HttpClient] fix reading the body after a ClientException … · symfony/symfony@36d382e · GitHub
[go: up one dir, main page]
Skip to content

Commit 36d382e

Browse files
nicolas-grekasnicolas-grekas
committed
bug #38530 [HttpClient] fix reading the body after a ClientException (nicolas-grekas)
This PR was merged into the 4.4 branch. Discussion ---------- [HttpClient] fix reading the body after a ClientException | Q | A | ------------- | --- | Branch? | 4.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | - | License | MIT | Doc PR | - #38508 wasn't enough to fix the issue. Commits ------- 0159a39 [HttpClient] fix reading the body after a ClientException
2 parents 6000e51 + 0159a39 commit 36d382e

File tree

6 files changed

+87
-50
lines changed

6 files changed

+87
-50
lines changed

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

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,40 @@
1+
<?php
2+
3+
/*
4+
* This file is part of the Symfony package.
5+
*
6+
* (c) Fabien Potencier <fabien@symfony.com>
7+
*
8+
* For the full copyright and license information, please view the LICENSE
9+
* file that was distributed with this source code.
10+
*/
11+
12+
namespace Symfony\Component\HttpClient\Internal;
13+
14+
/**
15+
* @author Nicolas Grekas <p@tchwork.com>
16+
*
17+
* @internal
18+
*/
19+
final class Canary
20+
{
21+
private $canceller;
22+
23+
public function __construct(\Closure $canceller)
24+
{
25+
$this->canceller = $canceller;
26+
}
27+
28+
public function cancel()
29+
{
30+
if (($canceller = $this->canceller) instanceof \Closure) {
31+
$this->canceller = null;
32+
$canceller();
33+
}
34+
}
35+
36+
public function __destruct()
37+
{
38+
$this->cancel();
39+
}
40+
}

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

Lines changed: 29 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515
use Symfony\Component\HttpClient\Chunk\FirstChunk;
1616
use Symfony\Component\HttpClient\Chunk\InformationalChunk;
1717
use Symfony\Component\HttpClient\Exception\TransportException;
18+
use Symfony\Component\HttpClient\Internal\Canary;
1819
use Symfony\Component\HttpClient\Internal\ClientState;
1920
use Symfony\Component\HttpClient\Internal\CurlClientState;
2021
use Symfony\Contracts\HttpClient\ResponseInterface;
@@ -149,6 +150,31 @@ public function __construct(CurlClientState $multi, $ch, array $options = null,
149150
// Schedule the request in a non-blocking way
150151
$multi->openHandles[$id] = [$ch, $options];
151152
curl_multi_add_handle($multi->handle, $ch);
153+
154+
$this->canary = new Canary(static function () use ($ch, $multi, $id) {
155+
unset($multi->openHandles[$id], $multi->handlesActivity[$id]);
156+
curl_setopt($ch, \CURLOPT_PRIVATE, '_0');
157+
158+
if (self::$performing) {
159+
return;
160+
}
161+
162+
curl_multi_remove_handle($multi->handle, $ch);
163+
curl_setopt_array($ch, [
164+
\CURLOPT_NOPROGRESS => true,
165+
\CURLOPT_PROGRESSFUNCTION => null,
166+
\CURLOPT_HEADERFUNCTION => null,
167+
\CURLOPT_WRITEFUNCTION => null,
168+
\CURLOPT_READFUNCTION => null,
169+
\CURLOPT_INFILE => null,
170+
]);
171+
172+
if (!$multi->openHandles) {
173+
// Schedule DNS cache eviction for the next request
174+
$multi->dnsCache->evictions = $multi->dnsCache->evictions ?: $multi->dnsCache->removals;
175+
$multi->dnsCache->removals = $multi->dnsCache->hostnames = [];
176+
}
177+
});
152178
}
153179

154180
/**
@@ -199,48 +225,11 @@ public function getContent(bool $throw = true): string
199225

200226
public function __destruct()
201227
{
202-
try {
203-
if (null === $this->timeout) {
204-
return; // Unused pushed response
205-
}
206-
207-
$this->doDestruct();
208-
} finally {
209-
$multi = clone $this->multi;
210-
211-
$this->close();
212-
213-
if (!$this->multi->openHandles) {
214-
// Schedule DNS cache eviction for the next request
215-
$this->multi->dnsCache->evictions = $this->multi->dnsCache->evictions ?: $this->multi->dnsCache->removals;
216-
$this->multi->dnsCache->removals = $this->multi->dnsCache->hostnames = [];
217-
}
218-
219-
$this->multi = $multi;
220-
}
221-
}
222-
223-
/**
224-
* {@inheritdoc}
225-
*/
226-
private function close(): void
227-
{
228-
unset($this->multi->openHandles[$this->id], $this->multi->handlesActivity[$this->id]);
229-
curl_setopt($this->handle, \CURLOPT_PRIVATE, '_0');
230-
231-
if (self::$performing) {
232-
return;
228+
if (null === $this->timeout) {
229+
return; // Unused pushed response
233230
}
234231

235-
curl_multi_remove_handle($this->multi->handle, $this->handle);
236-
curl_setopt_array($this->handle, [
237-
\CURLOPT_NOPROGRESS => true,
238-
\CURLOPT_PROGRESSFUNCTION => null,
239-
\CURLOPT_HEADERFUNCTION => null,
240-
\CURLOPT_WRITEFUNCTION => null,
241-
\CURLOPT_READFUNCTION => null,
242-
\CURLOPT_INFILE => null,
243-
]);
232+
$this->doDestruct();
244233
}
245234

246235
/**

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

Lines changed: 8 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414
use Psr\Log\LoggerInterface;
1515
use Symfony\Component\HttpClient\Chunk\FirstChunk;
1616
use Symfony\Component\HttpClient\Exception\TransportException;
17+
use Symfony\Component\HttpClient\Internal\Canary;
1718
use Symfony\Component\HttpClient\Internal\ClientState;
1819
use Symfony\Component\HttpClient\Internal\NativeClientState;
1920
use Symfony\Contracts\HttpClient\ResponseInterface;
@@ -43,7 +44,7 @@ final class NativeResponse implements ResponseInterface
4344
public function __construct(NativeClientState $multi, $context, string $url, array $options, array &$info, callable $resolveRedirect, ?callable $onProgress, ?LoggerInterface $logger)
4445
{
4546
$this->multi = $multi;
46-
$this->id = (int) $context;
47+
$this->id = $id = (int) $context;
4748
$this->context = $context;
4849
$this->url = $url;
4950
$this->logger = $logger;
@@ -63,6 +64,10 @@ public function __construct(NativeClientState $multi, $context, string $url, arr
6364
$this->initializer = static function (self $response) {
6465
return null === $response->remaining;
6566
};
67+
68+
$this->canary = new Canary(static function () use ($multi, $id) {
69+
unset($multi->openHandles[$id], $multi->handlesActivity[$id]);
70+
});
6671
}
6772

6873
/**
@@ -88,17 +93,11 @@ public function __destruct()
8893
try {
8994
$this->doDestruct();
9095
} finally {
91-
$multi = clone $this->multi;
92-
93-
$this->close();
94-
9596
// Clear the DNS cache when all requests completed
9697
if (0 >= --$this->multi->responseCount) {
9798
$this->multi->responseCount = 0;
9899
$this->multi->dnsCache = [];
99100
}
100-
101-
$this->multi = $multi;
102101
}
103102
}
104103

@@ -192,8 +191,8 @@ private function open(): void
192191
*/
193192
private function close(): void
194193
{
195-
unset($this->multi->openHandles[$this->id], $this->multi->handlesActivity[$this->id]);
196-
$this->handle = $this->buffer = $this->onProgress = null;
194+
$this->canary->cancel();
195+
$this->handle = $this->buffer = $this->inflate = $this->onProgress = null;
197196
}
198197

199198
/**

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

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,7 @@ trait ResponseTrait
3737
{
3838
private $logger;
3939
private $headers = [];
40+
private $canary;
4041

4142
/**
4243
* @var callable|null A callback that initializes the two previous properties
@@ -207,7 +208,11 @@ public function toStream(bool $throw = true)
207208
/**
208209
* Closes the response and all its network handles.
209210
*/
210-
abstract protected function close(): void;
211+
private function close(): void
212+
{
213+
$this->canary->cancel();
214+
$this->inflate = null;
215+
}
211216

212217
/**
213218
* Adds pending responses to the activity list.

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -134,6 +134,7 @@ public function testHandleIsRemovedOnException()
134134
// The response content-type mustn't be json as that calls getContent
135135
// @see src/Symfony/Component/HttpClient/Exception/HttpExceptionTrait.php:58
136136
$this->assertStringNotContainsString('json', $e->getResponse()->getHeaders(false)['content-type'][0] ?? '');
137+
unset($e);
137138

138139
$r = new \ReflectionProperty($client, 'multi');
139140
$r->setAccessible(true);

‎src/Symfony/Contracts/HttpClient/Test/Fixtures/web/index.php

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -66,6 +66,9 @@
6666
case '/404-gzipped':
6767
header('Content-Type: text/plain', true, 404);
6868
ob_start('ob_gzhandler');
69+
@ob_flush();
70+
flush();
71+
usleep(300000);
6972
echo 'some text';
7073
exit;
7174

0 commit comments

Comments
 (0)
0