Skip to content

[HttpClient] RetryableHttpClient + streamed responses fails to cancel responses properly #43391

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
Seldaek opened this issue Oct 11, 2021 · 1 comment

Comments

@Seldaek
Copy link
Member

Seldaek commented Oct 11, 2021

Symfony version(s) affected: 5.3.4

Description
When streaming responses and a response times out and gets canceled, it should then not throw anymore on __destruct. This works well without RetryableHttpClient, but when that wrapper is used it somehow still throws on __destruct.

How to reproduce

<?php

include 'vendor/autoload.php';

ini_set('error_reporting', -1);
ini_set('display_errors', true);

$client = Symfony\Component\HttpClient\HttpClient::create();
$client = new Symfony\Component\HttpClient\RetryableHttpClient($client);

$client = $client->withOptions([
    'timeout' => 0.1,
]);

$resp[] = $client->request('GET', 'http://api.github.com/repos/symfony/symfony');

foreach ($client->stream($resp) as $response => $chunk) {
    try {
        if ($chunk->isTimeout()) {
            $response->cancel();
            var_dump('RESP CANCELLED');
        }
    } catch (Symfony\Contracts\HttpClient\Exception\ExceptionInterface $e) {
        var_dump('CAUGHT', $e);
        die;
    }
}

var_dump('END, NOTHING CAUGHT, SHOULD NOT THROW');

Running this outputs:

string(14) "RESP CANCELLED"
./test.php:30:
string(37) "END, NOTHING CAUGHT, SHOULD NOT THROW"
PHP Fatal error:  Uncaught Symfony\Component\HttpClient\Exception\TimeoutException: Idle timeout reached for "http://api.github.com/repos/symfony/symfony". in ./vendor/symfony/http-client/Chunk/ErrorChunk.php:65

The fatal error here is not expected, and commenting out the RetryableHttpClient fixes it.

@miguelantoniosantos
Copy link

miguelantoniosantos commented Oct 11, 2021

As tested on HttpClientTestCase.php:838 if you add the following code after the foreach it will work without errors:

while ($response = array_shift($resp)) {
    try {
        unset($response);
    } catch (TransportExceptionInterface $e) {
    }
}

So, I believe, this is intended, if not the following code on TransportResponseTrait.php:140 and adjusting the unit tests should resolve this issue:

try {
    self::initialize($this, -0.0);
    $this->checkStatusCode();
} catch (TimeoutExceptionInterface $exception) {
    // ignore any timeout errors as we are destructing
}

nicolas-grekas added a commit that referenced this issue Oct 16, 2021
…nceled (nicolas-grekas)

This PR was merged into the 5.3 branch.

Discussion
----------

[HttpClient] fix RetryableHttpClient when a response is canceled

| Q             | A
| ------------- | ---
| Branch?       | 5.3
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | Fix #43390, fix #43391
| License       | MIT
| Doc PR        | -

Commits
-------

4f827c3 [HttpClient] fix RetryableHttpClient when a response is canceled
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants