Convert SocketException to guzzle ConnectException #3
Conversation
src/GuzzleHandlerAdapter.php
Outdated
| } catch (SocketException $e) { | ||
| $promise->reject(new ConnectException($e->getMessage(), $request, $e)); |
There was a problem hiding this comment.
I guess this is not always right, e.g. if a socket is closed while the request is sent, we likely don't want a ConnectException?
There was a problem hiding this comment.
Agreed, it might be better to handle this the same way as in CurlFactory.php, where only certain types of errors are converted to ConnectException, while all others are converted to RequestException.
But all raw SocketException should definitely be converted to either ConnectException or RequestException to be compatible with Guzzle, which always throws its own internal exceptions in such cases. Applications rely on these Guzzle exceptions, and they must remain consistent no matter what handler is used under the hood.
The only problem is that we cannot differentiate between SocketException cases. We need to add some form of differentiation to implement this properly.
See my related issue about this: amphp/http-client#379
There was a problem hiding this comment.
@kelunik
Could you please take another look at this PR?
I've made some update, now, timeout errors and DNS resolution errors (when possible to distinct them) are wrapped into ConnectException, while all other exceptions are wrapped into RequestException.
This behavior aligns with Guzzle default handlers, which also wrap exceptions into either ConnectException or RequestException.
For reference, see the implementations in CurlFactory and StreamHandler.
…on, all other exceptions to RequestException.
luzrain
force-pushed
the
connect_exception
branch
from
636688a to
8068f2c
Compare
Convert
Amp\Http\Client\SocketExceptionto guzzleGuzzleHttp\Exception\ConnectException