Improve exceptions thrown when an error happens during proxy CONNECT
#2721
Conversation
This file contains 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
idelpivnitskiy
commented
Oct 2, 2023
| package io.servicetalk.http.netty; | ||
|
|
||
| import io.servicetalk.transport.api.RetryableException; | ||
| package io.servicetalk.http.api; |
There was a problem hiding this comment.
This class was not released yet (introduced in #2711). Safe to move without going through deprecation process
Motivation: `ProxyResponseException` is marked as `RetryableException`. However, not all response statuses are retryable. For example, it doesn't make sense to auto-retry if proxy responds with 4xx (proxy auth required, bad request, etc.). Also, because proxy-related exceptions live in `servicetalk-http-netty`, gprc-api can not access them for better mapping into gRPC status code. Modifications: - Move `ProxyConnectException` from `http-netty` to `http-api`; - Deprecate `ProxyResponseException`, introduce `ProxyConnectResponseException` instead in `http-api`; - Use a retryable variant of `ProxyConnectResponseException` only for 5xx responses from a proxy server; - Include Channel info in every thrown `ProxyConnectException`; - Enhance `GrpcStatusException` to recognize `ProxyConnectResponseException`; - Extract `HttpResponseStatus` -> `GrpcStatusCode` mapping into `GrpcUtils.fromHttpStatus(...)` utility, add support for `PROXY_AUTHENTICATION_REQUIRED` status; - Enhance tests to verify expected behavior; Results: 1. `ProxyResponseException` and `ProxyConnectResponseException` are part of `http-api` now. 2. `http-netty` is careful when proxy related exceptions should be marked as `RetryableException` and when not. 3. grpc-api recognizes `ProxyConnectResponseException` and infers better grpc-status instead of always using `UNKNOWN`.
idelpivnitskiy
force-pushed
the
proxyExceptions
branch
from
28e8c24 to
0800209
Compare
daschl
reviewed
Oct 3, 2023
servicetalk-http-netty/src/main/java/io/servicetalk/http/netty/ProxyConnectChannelSingle.java
Outdated
Show resolved
Hide resolved
daschl
approved these changes
Oct 4, 2023
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.
Motivation:
ProxyResponseExceptionis marked asRetryableException. However, not all response statuses are retryable. For example, it doesn't make sense to auto-retry if proxy responds with 4xx (proxy auth required, bad request, etc.).Also, because proxy-related exceptions live in
servicetalk-http-netty, gprc-api can not access them for better mapping into gRPC status code.Modifications:
ProxyConnectExceptionfromhttp-nettytohttp-api;ProxyResponseException, introduceProxyConnectResponseExceptioninstead inhttp-api;ProxyConnectResponseExceptiononly for specific status codes from a proxy server are are intended to be retryable;ProxyConnectException;GrpcStatusExceptionto recognizeProxyConnectResponseException;HttpResponseStatus->GrpcStatusCodemapping intoGrpcUtils.fromHttpStatus(...)utility, add support forPROXY_AUTHENTICATION_REQUIREDstatus;Results:
ProxyResponseExceptionandProxyConnectResponseExceptionare part ofhttp-apinow.http-nettyis careful when proxy related exceptions should be marked asRetryableExceptionand when not.ProxyConnectResponseExceptionand infers better grpc-status instead of always usingUNKNOWN.