Ensure CURLE_RECV_ERROR (56) throws ClientException #43197
Conversation
|
Hey! To help keep things organized, we don't allow "Draft" pull requests. Could you please click the "ready for review" button or close this PR and open a new one when you are done? Note that a pull request does not have to be "perfect" or "ready for merge" when you first open it. We just want it to be ready for a first review. Cheers! Carsonbot |
|
I don't think this is the correct patch. What might happen is that the proxy responds with a 407 and then it abruptly closes the connection. The 407 is recorded by the client, but the unexpectedly closed connection takes over and generates this TransportException. In order to ignore the RECV error, we need to check other states of the client when it happens. |
|
Thank you @Nicals for the speedy feedback. The values requested were dumped just before the updated line. Use case: Use authenticated proxy with incorrect or blank username and password: Response headers from proxy: Requested trace data: Use case: Use authenticated proxy with correct username and password: |
|
Thanks. How did you get the first dump, the one that mentions |
|
I kept my changes as per the PR and used this code to initiate the request: $response = $this->httpClient->request('POST', $url, $requestOptions);
$responseCode = $response->getStatusCode();
if (200 !== $responseCode) {
$this->logger->error('response code', [$responseCode]);
$this->logger->error('response info', [$response->getInfo()]);
}The PR results in the 407 resulting in a ClientException instead of a TransportException and therefore the response info in the response is populated with the actual headers from the proxy. Without the patch, there is no way to retrieve the actual headers returned by the proxy. |
|
Can you please try the following patch and let me know how it works? --- a/src/Symfony/Component/HttpClient/Response/CurlResponse.php
+++ b/src/Symfony/Component/HttpClient/Response/CurlResponse.php
@@ -313,6 +313,10 @@ final class CurlResponse implements ResponseInterface, StreamableInterface
}
}
+ if (\CURLE_RECV_ERROR === $result && 400 <= ($responses[(int) $ch]->info['http_code'] ?? 0)) {
+ $multi->handlesActivity[$id][] = new FirstChunk();
+ }
+
$multi->handlesActivity[$id][] = null;Also, can you give a try to how AmpHttpClient behaves in the same situation? |
|
Thank you for the quick response. I removed my patch and applied yours and that worked. The response code back to my application is 407 and the proxy response headers are now populated in It also handled a Gateway time-out (504 error) as well as a fully authenticated and working request. From what I can tell, your patch is indeed fixing the problem I experienced. As for AmpHttpClient, I will have to do some more testing and provide feedback on that at a later stage. Anything you would like me to do with this PR? Should I update it to reflect your patch, or will you file a new one? |
|
Please update this PR, that's fine. I'm just not sure if/how this can be tested. |
|
Closing in favor of #43243 |
|
Its a pleasure. |
Fix issue #42806
CURLE_RECV_ERROR (56) caused by proxy throws an TransportException instead of ClientException preventing further debugging