Repository navigation
fix: handle Guzzle 8 network and response timeout exceptions - #458
Merged
Merged
Conversation
Guzzle 8 no longer reports every transport failure as ConnectException. A timeout before response headers is NetworkTimeoutException, cURL 52/55/56 are NetworkException, and a timeout after response headers is ResponseTimeoutException. The first two extend TransferException without being a ConnectException or RequestException, so they escaped HttpClient::request() unretried and unmapped. The third was mapped to a BaseRequestException carrying the status code and partial body of the interrupted response. On Guzzle 7 all of these were ConnectException. Catch PSR-18's NetworkExceptionInterface (implemented by ConnectException on Guzzle 7 and by NetworkException on Guzzle 8) and ResponseTimeoutException in the ConnectException branch, and classify both timeout types as TimeoutException. On Guzzle 7 the caught set is unchanged: ConnectException is the only NetworkExceptionInterface implementation there.
Contributor
 [Medium impact] The PR appears safe to merge; the previous test coverage issue is fixed. SummaryThe PR retries Guzzle 8 network and response timeout failures and maps them to the SDK’s connection or timeout exceptions.
Reviews (2) · Last reviewed commit: "test: pin timeout classification to the ..." · Reviewed by Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Both timeout tests used messages containing "timed out", which the message fallback in isTimeoutException() matches on its own. Use messages without it so the tests fail if the NetworkTimeoutException / ResponseTimeoutException checks are removed.
Contributor
|
thanks ! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Since #427 the SDK allows Guzzle 8, but
HttpClient::request()still catches onlyConnectExceptionandRequestException. Guzzle 8 classifies transport failures more finely (CurlFactory::createRejection()), so some of them now reach the caller unretried and unmapped:ConnectExceptionNetworkTimeoutExceptionConnectException(52) orRequestExceptionwithout a response (55, 56)NetworkExceptionConnectExceptionResponseTimeoutExceptionBaseRequestExceptionwith the interrupted response's status (e.g. 200) and its partial body as the messageNetworkExceptionandNetworkTimeoutExceptionextendTransferExceptionwithout being aConnectExceptionorRequestException, so neither catch matches them.ResponseTimeoutExceptionis aResponseException, so it is mapped as an API error.This PR:
NetworkExceptionInterfaceandResponseTimeoutExceptionin the branch that used to catchConnectException, so these failures are retried and mapped toConnectionException/TimeoutExceptionas they were on Guzzle 7.ConnectExceptionimplementsNetworkExceptionInterfaceon both majors, and on Guzzle 7 it is the only implementation, so the caught set on Guzzle 7 is unchanged.NetworkTimeoutExceptionandResponseTimeoutExceptionasTimeoutException. (The'timed out'message fallback already matched cURL's text; this makes it explicit, like the existingConnectTimeoutExceptioncheck.)Retrying
ResponseTimeoutExceptionmatches what the SDK did on Guzzle 7, where the same failure was aConnectException.HandlerClosedException(also a bareTransferExceptionon Guzzle 8) is left as is.Tests: four cases in
tests/HttpClientTest.php(timeout before headers, timeout after headers, network failure, network failure retried). They skip on Guzzle 7, where these classes do not exist../script/cipasses on Guzzle 8.3.0 (php-cs-fixer, PHPStan, 395 tests), and the suite passes on Guzzle 7.16.0 with the four new tests skipped. Without the fix, the four tests fail on Guzzle 8.Documentation
Does this require changes to the WorkOS Docs? E.g. the API Reference or code snippets need updates.
If yes, link a related docs PR and add a docs maintainer as a reviewer. Their approval is required.