Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix ExchangeType detection for HeaderOverridingHttpRequest. #5787

Merged
merged 1 commit into from
Jun 27, 2024
Merged

Fix ExchangeType detection for HeaderOverridingHttpRequest. #5787

merged 1 commit into from
Jun 27, 2024

Conversation

minwoox
Copy link
Contributor

@minwoox minwoox commented Jun 26, 2024

Motivation:
The RetryingClient uses ExchangeType to decide whether to use HttpRequestDuplicator:

armeria/core/src/main/java/com/linecorp/armeria/client/retry/RetryingClient.java

Lines 245 to 246 in 7474525

if (ctx.exchangeType().isRequestStreaming()) {
final HttpRequestDuplicator reqDuplicator = req.toDuplicator(ctx.eventLoop().withoutContext(), 0);
Thus, setting the proper ExchangeType is important.

ExchangeType is currently inferred from the HttpRequest implementation:

armeria/core/src/main/java/com/linecorp/armeria/internal/client/DefaultClientRequestContext.java

Lines 285 to 288 in 7474525

if (req instanceof FixedStreamMessage) {
return ExchangeType.RESPONSE_STREAMING;
}
return ExchangeType.BIDI_STREAMING;

However, DefaultWebClient wraps the request, which results in incorrect ExchangeType detection:

armeria/core/src/main/java/com/linecorp/armeria/client/DefaultWebClient.java

Line 113 in 7474525

newReq = req.withHeaders(req.headers().toBuilder().path(newPath));

Modifications:

  • Unwrapped HeaderOverridingHttpRequest to detect the correct ExchangeType.
  • Added RequestOptions when sending a request where applicable.

Result:

  • The ExchangeType is now correctly detected for the default WebClient.

@minwoox
Fix ExchangeType detection for HeaderOverridingHttpRequest.
13d2e78 
Motivation:
The `RetryingClient` uses `ExchangeType` to decide whether to use `HttpRequestDuplicator`:
https://github.com/line/armeria/blob/7474525b8cf25f02be6df7c38510a8fb6a88cb1f/core/src/main/java/com/linecorp/armeria/client/retry/RetryingClient.java#L245-L246
Thus, setting the proper `ExchangeType` is important.

`ExchangeType` is currently inferred from the `HttpRequest` implementation:
https://github.com/line/armeria/blob/7474525b8cf25f02be6df7c38510a8fb6a88cb1f/core/src/main/java/com/linecorp/armeria/internal/client/DefaultClientRequestContext.java#L285-L288

However, `DefaultWebClient` wraps the request, which results in incorrect `ExchangeType` detection:
https://github.com/line/armeria/blob/7474525b8cf25f02be6df7c38510a8fb6a88cb1f/core/src/main/java/com/linecorp/armeria/client/DefaultWebClient.java#L113

Modifications:
- Unwrapped `HeaderOverridingHttpRequest` to detect the correct `ExchangeType`.
- Added `RequestOptions` when sending a request where applicable.

Result:
- The `ExchangeType` is now correctly detected for the default `WebClient`.
@minwoox minwoox added this to the 1.29.1 milestone Jun 26, 2024
trustin
trustin approved these changes Jun 26, 2024
View reviewed changes
Copy link
Member

@trustin trustin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @minwoox !

jrhee17
jrhee17 approved these changes Jun 26, 2024
View reviewed changes
Copy link
Contributor

@jrhee17 jrhee17 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 👍 👍

@github-actions GitHub Actions
Copy link
Contributor

🔍 Build Scan® (commit: 13d2e78)

Job name Status Build Scan®
build-windows-latest-jdk-21 ✅ https://ge.armeria.dev/s/tnen66ztoajpk
build-self-hosted-unsafe-jdk-8 ✅ https://ge.armeria.dev/s/s3smti2472zpw
build-self-hosted-unsafe-jdk-21-snapshot-blockhound ✅ https://ge.armeria.dev/s/3ypbyjy3wpq7q
build-self-hosted-unsafe-jdk-17-min-java-17-coverage ✅ https://ge.armeria.dev/s/p7ny7prr2ylym
build-self-hosted-unsafe-jdk-17-min-java-11 ✅ https://ge.armeria.dev/s/mnpfa7ykyxgse
build-self-hosted-unsafe-jdk-17-leak ✅ https://ge.armeria.dev/s/hcuxnxj7kp2ro
build-self-hosted-unsafe-jdk-11 ✅ https://ge.armeria.dev/s/grkv4o2wrtlsg
build-macos-12-jdk-21 ✅ https://ge.armeria.dev/s/nduuzlqzrmlvm

ikhoon
ikhoon approved these changes Jun 27, 2024
View reviewed changes
Copy link
Contributor

@ikhoon ikhoon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! 👍 👍

oauth2/src/main/java/com/linecorp/armeria/internal/common/auth/oauth2/OAuth2Endpoint.java
@@ -43,7 +50,7 @@ public OAuth2Endpoint(WebClient endpoint, String endpointPath,
public CompletableFuture<T> execute(OAuth2Request oAuth2Request) {
final HttpRequest request = oAuth2Request.asHttpRequest(endpointPath);
final QueryParams requestParams = oAuth2Request.bodyParams();
return endpoint.execute(request)
return endpoint.execute(request, UNARY_REQUEST_OPTIONS)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@minwoox minwoox merged commit 369614f into line:main Jun 27, 2024
15 checks passed
@minwoox minwoox deleted the unary_client branch June 27, 2024 11:04
minwoox added a commit that referenced this pull request Jun 28, 2024
@minwoox
Fix ExchangeType detection for HeaderOverridingHttpRequest. (#5787)
a48670e 
Motivation:
The `RetryingClient` uses `ExchangeType` to decide whether to use `HttpRequestDuplicator`: https://github.com/line/armeria/blob/7474525b8cf25f02be6df7c38510a8fb6a88cb1f/core/src/main/java/com/linecorp/armeria/client/retry/RetryingClient.java#L245-L246 Thus, setting the proper `ExchangeType` is important.

`ExchangeType` is currently inferred from the `HttpRequest` implementation: https://github.com/line/armeria/blob/7474525b8cf25f02be6df7c38510a8fb6a88cb1f/core/src/main/java/com/linecorp/armeria/internal/client/DefaultClientRequestContext.java#L285-L288

However, `DefaultWebClient` wraps the request, which results in incorrect `ExchangeType` detection: https://github.com/line/armeria/blob/7474525b8cf25f02be6df7c38510a8fb6a88cb1f/core/src/main/java/com/linecorp/armeria/client/DefaultWebClient.java#L113

Modifications:
- Unwrapped `HeaderOverridingHttpRequest` to detect the correct `ExchangeType`.
- Added `RequestOptions` when sending a request where applicable.

Result:
- The `ExchangeType` is now correctly detected for the default `WebClient`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jrhee17 jrhee17 jrhee17 approved these changes

@ikhoon ikhoon ikhoon approved these changes

@trustin trustin trustin approved these changes

Assignees
No one assigned
Labels
improvement
Projects
None yet
Milestone
1.29.1
Development

Successfully merging this pull request may close these issues.
4 participants
@minwoox @trustin @ikhoon @jrhee17