Unresolved async response throws on destruction when it has 3xx #48924
Replies: 4 comments
-
I may be misunderstanding the issue description, but this sounds to me like what is documented at https://symfony.com/doc/current/http_client.html#handling-exceptions. Can you double-check and clarify what you mean if you disagree? :) |
Beta Was this translation helpful? Give feedback.
-
throwing on non-successful responses in The issue in your code is that you don't handle status code checks yourselves for all the responses as you have an early return in your loop. And so the default error handling logic kicks in. |
Beta Was this translation helpful? Give feedback.
-
Thanks for the feedback, and the docs pointer!I really have to wrap my head around all this async, be it promises, fibers, or this implementation, and now i understand better what the docs want to tell me. I think i have solved most of this, but there's one thing that still stumbles me. According to Dealing with Network Errors, FTR, the resulting code that i am happy with (except for the open question) public function resolveRedirects(UrlList $urlList): UrlRedirectInfoList {
$urlRedirectInfoList = new UrlRedirectInfoList();
$responses = [];
foreach ($urlList as $urlItem) {
$url = $urlItem->toString();
if (!isset($responses[$url])) {
$responses[$url] = $this->client->request('GET', $url);
}
}
// Pass in timeout only now to not get exceptions too early.
foreach ($this->client->stream($responses, $this->timeoutSeconds) as $response => $chunk) {
try {
if ($chunk->isTimeout()) {
$response->cancel();
} elseif ($chunk->isFirst()) {
$url = array_search($response, $responses, TRUE);
// Headers arrived.
$redirectUrls = [];
$isRedirectResponse = $response->getStatusCode() >= 300 && $response->getStatusCode() < 400;
/** @noinspection PhpUnhandledExceptionInspection */
$redirectUrl = $isRedirectResponse ? $response->getHeaders(throw: FALSE)['location'][0] ?? NULL : NULL;;
if (!empty($redirectUrl)) {
$redirectUrls[] = $redirectUrl;
$urlRedirectInfo = new UrlRedirectInfo($url, ...$redirectUrls);
$urlRedirectInfoList->add($urlRedirectInfo);
// In this version, intentionally do NOT follow redirections further than
// one level.
}
// We don't need more.
$response->cancel();
} elseif ($chunk->isLast()) {
// We will never get here.
}
} catch (TransportExceptionInterface $e) {
// @todo Log.
}
}
return $urlRedirectInfoList;
} FTR(2), the obsoleted code that has a @ fixme comment showing my initial train of thought public function resolveRedirects(UrlList $urlList): UrlRedirectInfoList {
$urlRedirectInfoList = new UrlRedirectInfoList();
// Span a try-catch around the complete lifetime of the async responses,
// to not miss any exception.
try {
// Place all requests.
$responses = [];
// The "place-many-requests" foreach loop.
foreach ($urlList as $urlItem) {
$url = $urlItem->toString();
if (!isset($responses[$url])) {
$responses[$url] = $this->client->request('GET', $url);
}
}
// The "work-down-all-responses" foreach loop.
foreach ($responses as $url => $response) {
// Span a try-catch around the response evaluation loop. So if a
// response throws in that loop, processing continues for the next
// response like we want it.
// @fixme Assume an async response throws an exception **before**
// entering the foreach loop, or in the microsecond when looping.
// Then the exception is caught by the outer try-catch, dumping all
// remaining responses, which is not what we want.
// The problem is more general: Given multiple responses, to catch
// an exception for one response, the try clause is left and another
// response can throw an exception. Unresolvable.
// The above first approximation fortunately is only true in a world
// with true parallelism. Afaik we don't have true parallelism, bot
// fibers can only be switched (and the exception thrown) with an
// explicit switch or sleep or an i/o operation. Leveraging that
// knowledge, this for-loop is safe as long as we avoid any i/o
// outside the try-catch.
// But there's still the problem with the timespan **before** entering
// the work-down-all-responses foreach loop. While quite unprobable
// (bot more probable the more requests), it is perfectly possible
// that one of the responses throws timeout (or fwiw another exception),
// while a second one is placed, when the i/o operation of placing the
// second requests triggers the fiber event loop.
// How can we handle **that** situation?
// Anyway, it looks like event in this simplest use case of placing
// multiple events, catching exceptions needs a thorough knowledge
// of when and how the event loop can be triggered, and where the
// exception can be thrown.
try {
$redirectUrls = [];
$isRedirectResponse = $response->getStatusCode() >= 300 && $response->getStatusCode() < 400;
$redirectUrl = $isRedirectResponse ? $response->getHeaders(throw: FALSE)['location'][0] ?? NULL : NULL;;
if (!empty($redirectUrl)) {
$redirectUrls[] = $redirectUrl;
$urlRedirectInfo = new UrlRedirectInfo($url, ...$redirectUrls);
$urlRedirectInfoList->add($urlRedirectInfo);
// In this version, intentionally do NOT follow redirections further than
// one level.
}
} catch (TimeoutException $e) {
// @todo Log.
}
}
// Ensure destructor is in try-catch too. Otherwise, a response that is
// header-complete, but not complete yet, might throw timeout after
// leaving this scope, even if it's only a microsecond away from
// destruction by leaving the function.
unset($responses);
} catch (TimeoutException $e) {
// @todo Log.
}
return $urlRedirectInfoList;
} |
Beta Was this translation helpful? Give feedback.
-
The first code sample is obviously better as it will take network order into account and won't choke when one response times out. |
Beta Was this translation helpful? Give feedback.
Uh oh!
There was an error while loading. Please reload this page.
-
Symfony version(s) affected
6.2.2
Description
I need to resolve a redirect without actually requesting the redirect location (because that one performs an action, while against the spec, it is a common pattern in the wild, e.g. newsletter unsubscribe links).
When i limit max_redirects to 0, and use
$resoponse->headers(throw: false)
, everythong works out fine. For days and months.Until the response has had any eror, e.g. a timeout.
Then i get "Symfony\Component\HttpClient\Exception\RedirectionException in CommonResponseTrait.php:169".
It turns out the destructor of AsyncResponse - contrary to TraceableResponse, and contrary to getHeaders - always throws on 3xx.
Maybe this also happens under other circumstances, i did not check that.
How to reproduce
Possible Solution
In AsyncResponse::__destruct:180, change
$this->getHeaders(throw: true)
to$this->getHeaders(throw: false)
.Additional Context
No response
Beta Was this translation helpful? Give feedback.
All reactions