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

Introduce a way to unwrap RequestContexts #4308

Merged
merged 26 commits into from
Aug 4, 2022
Merged

Introduce a way to unwrap RequestContexts #4308

Show file tree
Hide file tree
Changes from 23 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
0ae3cc8
make requestcontext unwrappable
jrhee17 May 27, 2022
652a16f
modify unwrap to peel only a single layer
jrhee17 May 31, 2022
88faa43
also add a test for client unwrapping
jrhee17 Jun 22, 2022
0230f5f
introduce extension classes for request context classes
jrhee17 Jun 22, 2022
c6dccec
add test case for service as
jrhee17 Jun 22, 2022
c2a203d
modify inheritance hierarchy
jrhee17 Jun 22, 2022
0366c8e
fix compile error
jrhee17 Jun 22, 2022
a4da669
try moving DefaultClientRequestContext to internal
jrhee17 Jun 22, 2022
a5c51f9
move the other abstract implementations to internal
jrhee17 Jun 23, 2022
aafa8f4
minor cleanups
jrhee17 Jun 23, 2022
720a942
remove unnecessary unstableapi
jrhee17 Jun 24, 2022
7c31e56
Merge branch 'master' of github.com:line/armeria into feature/unwrapp…
jrhee17 Jul 1, 2022
0d867c8
clientExtension also extends extension
jrhee17 Jul 1, 2022
d623c2f
Merge branch 'master' of github.com:line/armeria into feature/unwrapp…
jrhee17 Jul 1, 2022
2dbabe9
Merge branch 'master' of github.com:line/armeria into feature/unwrapp…
jrhee17 Jul 5, 2022
e099f31
retrying client now unwraps instead of force casting
jrhee17 Jul 6, 2022
86a3871
Merge branch 'master' of github.com:line/armeria into feature/unwrapp…
jrhee17 Jul 6, 2022
259e64d
remove another instance of force cast
jrhee17 Jul 6, 2022
986662d
address comments by @ikhoon
jrhee17 Jul 6, 2022
50aa23d
address lint failures
jrhee17 Jul 6, 2022
48d4c27
Merge branch 'master' of github.com:line/armeria into feature/unwrapp…
jrhee17 Jul 13, 2022
061c46b
address comments by @minwoox
jrhee17 Jul 18, 2022
3e6c62b
Merge branch 'master' into pr-4308@jrhee17+feature/unwrappable-context
ikhoon Aug 4, 2022
2dc4433
override unwrapAll for RequestContextWrapper
jrhee17 Aug 4, 2022
8998f91
RequestContext also overrides unwrapAll
jrhee17 Aug 4, 2022
32d7692
override unwrapAll return types
jrhee17 Aug 4, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions brave/src/main/java/com/linecorp/armeria/client/brave/BraveClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@
import org.slf4j.LoggerFactory;

import com.linecorp.armeria.client.ClientRequestContext;
import com.linecorp.armeria.client.DefaultClientRequestContext;
import com.linecorp.armeria.client.HttpClient;
import com.linecorp.armeria.client.SimpleDecoratingHttpClient;
import com.linecorp.armeria.common.HttpRequest;
Expand All @@ -34,6 +33,7 @@
import com.linecorp.armeria.common.annotation.Nullable;
import com.linecorp.armeria.common.brave.RequestContextCurrentTraceContext;
import com.linecorp.armeria.common.logging.ClientConnectionTimings;
import com.linecorp.armeria.internal.common.RequestContextExtension;
import com.linecorp.armeria.internal.common.brave.SpanTags;

import brave.Span;
Expand Down Expand Up @@ -121,10 +121,10 @@ public HttpResponse execute(ClientRequestContext ctx, HttpRequest req) throws Ex
req = req.withHeaders(newHeaders);
ctx.updateRequest(req);

if (currentTraceContext != null && !span.isNoop() && ctx instanceof DefaultClientRequestContext) {
final DefaultClientRequestContext defaultCtx = (DefaultClientRequestContext) ctx;
final RequestContextExtension ctxExtension = ctx.as(RequestContextExtension.class);
if (currentTraceContext != null && !span.isNoop() && ctxExtension != null) {
// Make the span the current span and run scope decorators when the ctx is pushed.
defaultCtx.hook(() -> currentTraceContext.newScope(span.context()));
ctxExtension.hook(() -> currentTraceContext.newScope(span.context()));
}

maybeAddTagsToSpan(ctx, braveReq, span);
Expand Down
11 changes: 5 additions & 6 deletions brave/src/main/java/com/linecorp/armeria/server/brave/BraveService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,8 @@
import com.linecorp.armeria.common.HttpRequest;
import com.linecorp.armeria.common.HttpResponse;
import com.linecorp.armeria.common.brave.RequestContextCurrentTraceContext;
import com.linecorp.armeria.internal.common.RequestContextExtension;
import com.linecorp.armeria.internal.common.brave.SpanTags;
import com.linecorp.armeria.server.DefaultServiceRequestContext;
import com.linecorp.armeria.server.HttpService;
import com.linecorp.armeria.server.ServiceRequestContext;
import com.linecorp.armeria.server.SimpleDecoratingHttpService;
Expand Down Expand Up @@ -100,12 +100,11 @@ public HttpResponse serve(ServiceRequestContext ctx, HttpRequest req) throws Exc
final HttpServerRequest braveReq = ServiceRequestContextAdapter.asHttpServerRequest(ctx);
final Span span = handler.handleReceive(braveReq);

if (currentTraceContext.scopeDecoratorAdded() && !span.isNoop() &&
ctx instanceof DefaultServiceRequestContext) {
final DefaultServiceRequestContext defaultCtx = (DefaultServiceRequestContext) ctx;
final RequestContextExtension ctxExtension = ctx.as(RequestContextExtension.class);
if (currentTraceContext.scopeDecoratorAdded() && !span.isNoop() && ctxExtension != null) {
// Run the scope decorators when the ctx is pushed to the thread local.
defaultCtx.hook(() -> currentTraceContext.decorateScope(span.context(),
SERVICE_REQUEST_DECORATING_SCOPE));
ctxExtension.hook(() -> currentTraceContext.decorateScope(span.context(),
SERVICE_REQUEST_DECORATING_SCOPE));
}

maybeAddTagsToSpan(ctx, braveReq, span);
Expand Down
5 changes: 5 additions & 0 deletions core/src/main/java/com/linecorp/armeria/client/ClientRequestContext.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -534,4 +534,9 @@ default void timeoutNow() {
@Override
@UnstableApi
ExchangeType exchangeType();

@Override
default ClientRequestContext unwrap() {
return (ClientRequestContext) RequestContext.super.unwrap();
}
}
1 change: 1 addition & 0 deletions core/src/main/java/com/linecorp/armeria/client/ClientRequestContextBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
import com.linecorp.armeria.common.annotation.Nullable;
import com.linecorp.armeria.common.logging.ClientConnectionTimings;
import com.linecorp.armeria.common.util.SystemInfo;
import com.linecorp.armeria.internal.client.DefaultClientRequestContext;
import com.linecorp.armeria.internal.common.CancellationScheduler;
import com.linecorp.armeria.internal.common.CancellationScheduler.CancellationTask;

Expand Down
1 change: 1 addition & 0 deletions core/src/main/java/com/linecorp/armeria/client/Clients.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
import com.linecorp.armeria.common.annotation.Nullable;
import com.linecorp.armeria.common.util.SafeCloseable;
import com.linecorp.armeria.common.util.Unwrappable;
import com.linecorp.armeria.internal.client.ClientThreadLocalState;

/**
* Creates a new client that connects to a specified {@link URI}.
Expand Down
13 changes: 1 addition & 12 deletions core/src/main/java/com/linecorp/armeria/client/DefaultWebClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,14 +16,13 @@

package com.linecorp.armeria.client;

import static com.linecorp.armeria.internal.client.ClientUtil.pathWithQuery;
import static com.linecorp.armeria.internal.common.ArmeriaHttpUtil.concatPaths;
import static com.linecorp.armeria.internal.common.ArmeriaHttpUtil.isAbsoluteUri;
import static java.util.Objects.requireNonNull;

import java.net.URI;

import com.google.common.base.Strings;

import com.linecorp.armeria.client.endpoint.EndpointGroup;
import com.linecorp.armeria.common.ExchangeType;
import com.linecorp.armeria.common.HttpRequest;
Expand Down Expand Up @@ -150,14 +149,4 @@ public RestClient asRestClient() {
public HttpClient unwrap() {
return (HttpClient) super.unwrap();
}

static String pathWithQuery(URI uri, @Nullable String query) {
String path = uri.getRawPath();
if (Strings.isNullOrEmpty(path)) {
path = query == null ? "/" : "/?" + query;
} else if (query != null) {
path = path + '?' + query;
}
return path;
}
}
17 changes: 12 additions & 5 deletions core/src/main/java/com/linecorp/armeria/client/HttpResponseDecoder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
import com.linecorp.armeria.common.stream.CancelledSubscriptionException;
import com.linecorp.armeria.common.stream.StreamWriter;
import com.linecorp.armeria.common.util.Exceptions;
import com.linecorp.armeria.internal.client.ClientRequestContextExtension;
import com.linecorp.armeria.internal.common.CancellationScheduler;
import com.linecorp.armeria.internal.common.CancellationScheduler.CancellationTask;
import com.linecorp.armeria.internal.common.InboundTrafficController;
Expand Down Expand Up @@ -354,9 +355,11 @@ private void cancelAction(@Nullable Throwable cause) {
private void cancelTimeoutOrLog(@Nullable Throwable cause, boolean cancel) {

CancellationScheduler responseCancellationScheduler = null;
if (ctx instanceof DefaultClientRequestContext) {
responseCancellationScheduler =
((DefaultClientRequestContext) ctx).responseCancellationScheduler();
if (ctx != null) {
final ClientRequestContextExtension ctxExtension = ctx.as(ClientRequestContextExtension.class);
if (ctxExtension != null) {
responseCancellationScheduler = ctxExtension.responseCancellationScheduler();
}
}

if (responseCancellationScheduler == null || !responseCancellationScheduler.isFinished()) {
Expand Down Expand Up @@ -398,9 +401,13 @@ private void cancelTimeoutOrLog(@Nullable Throwable cause, boolean cancel) {
}

void initTimeout() {
if (ctx instanceof DefaultClientRequestContext) {
if (ctx == null) {
return;
}
final ClientRequestContextExtension ctxExtension = ctx.as(ClientRequestContextExtension.class);
if (ctxExtension != null) {
final CancellationScheduler responseCancellationScheduler =
((DefaultClientRequestContext) ctx).responseCancellationScheduler();
ctxExtension.responseCancellationScheduler();
responseCancellationScheduler.init(
ctx.eventLoop(), newCancellationTask(),
TimeUnit.MILLISECONDS.toNanos(responseTimeoutMillis), /* server */ false);
Expand Down
1 change: 1 addition & 0 deletions core/src/main/java/com/linecorp/armeria/client/UserClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
import com.linecorp.armeria.common.annotation.Nullable;
import com.linecorp.armeria.common.util.AbstractUnwrappable;
import com.linecorp.armeria.common.util.SystemInfo;
import com.linecorp.armeria.internal.client.DefaultClientRequestContext;

import io.micrometer.core.instrument.MeterRegistry;

Expand Down
11 changes: 6 additions & 5 deletions core/src/main/java/com/linecorp/armeria/client/retry/RetryingClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@
import org.slf4j.LoggerFactory;

import com.linecorp.armeria.client.ClientRequestContext;
import com.linecorp.armeria.client.DefaultClientRequestContext;
import com.linecorp.armeria.client.HttpClient;
import com.linecorp.armeria.client.ResponseTimeoutException;
import com.linecorp.armeria.client.endpoint.EndpointGroup;
Expand All @@ -49,6 +48,7 @@
import com.linecorp.armeria.common.stream.AbortedStreamException;
import com.linecorp.armeria.internal.client.AggregatedHttpRequestDuplicator;
import com.linecorp.armeria.internal.client.ClientPendingThrowableUtil;
import com.linecorp.armeria.internal.client.ClientRequestContextExtension;
import com.linecorp.armeria.internal.client.TruncatingHttpResponse;

import io.netty.handler.codec.DateFormatter;
Expand Down Expand Up @@ -309,14 +309,15 @@ private void doExecute0(ClientRequestContext ctx, HttpRequestDuplicator rootReqD

final HttpResponse response;
final EndpointGroup endpointGroup = derivedCtx.endpointGroup();
if (!initialAttempt && derivedCtx instanceof DefaultClientRequestContext &&
final ClientRequestContextExtension ctxExtension = derivedCtx.as(ClientRequestContextExtension.class);
if (!initialAttempt && ctxExtension != null &&
endpointGroup != null && derivedCtx.endpoint() == null) {
// clear the pending throwable to retry endpoint selection
ClientPendingThrowableUtil.removePendingThrowable(derivedCtx);
// if the endpoint hasn't been selected, try to initialize the ctx with a new endpoint/event loop
final DefaultClientRequestContext casted = (DefaultClientRequestContext) derivedCtx;
response = initContextAndExecuteWithFallback(unwrap(), casted, endpointGroup, HttpResponse::from,
(context, cause) -> HttpResponse.ofFailure(cause));
response = initContextAndExecuteWithFallback(
unwrap(), ctxExtension, endpointGroup, HttpResponse::from,
(context, cause) -> HttpResponse.ofFailure(cause));
} else {
response = executeWithFallback(unwrap(), derivedCtx,
(context, cause) -> HttpResponse.ofFailure(cause));
Expand Down
8 changes: 4 additions & 4 deletions core/src/main/java/com/linecorp/armeria/client/retry/RetryingRpcClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@
import java.util.function.Function;

import com.linecorp.armeria.client.ClientRequestContext;
import com.linecorp.armeria.client.DefaultClientRequestContext;
import com.linecorp.armeria.client.ResponseTimeoutException;
import com.linecorp.armeria.client.RpcClient;
import com.linecorp.armeria.client.endpoint.EndpointGroup;
Expand All @@ -32,6 +31,7 @@
import com.linecorp.armeria.common.RpcRequest;
import com.linecorp.armeria.common.RpcResponse;
import com.linecorp.armeria.internal.client.ClientPendingThrowableUtil;
import com.linecorp.armeria.internal.client.ClientRequestContextExtension;
import com.linecorp.armeria.internal.common.util.StringUtil;

/**
Expand Down Expand Up @@ -172,14 +172,14 @@ private void doExecute0(ClientRequestContext ctx, RpcRequest req,

final RpcResponse res;

final ClientRequestContextExtension ctxExtension = derivedCtx.as(ClientRequestContextExtension.class);
final EndpointGroup endpointGroup = derivedCtx.endpointGroup();
if (!initialAttempt && derivedCtx instanceof DefaultClientRequestContext &&
if (!initialAttempt && ctxExtension != null &&
endpointGroup != null && derivedCtx.endpoint() == null) {
// clear the pending throwable to retry endpoint selection
ClientPendingThrowableUtil.removePendingThrowable(derivedCtx);
// if the endpoint hasn't been selected, try to initialize the ctx with a new endpoint/event loop
final DefaultClientRequestContext casted = (DefaultClientRequestContext) derivedCtx;
res = initContextAndExecuteWithFallback(unwrap(), casted, endpointGroup, RpcResponse::from,
res = initContextAndExecuteWithFallback(unwrap(), ctxExtension, endpointGroup, RpcResponse::from,
(context, cause) -> RpcResponse.ofFailure(cause));
} else {
res = executeWithFallback(unwrap(), derivedCtx,
Expand Down
8 changes: 7 additions & 1 deletion core/src/main/java/com/linecorp/armeria/common/RequestContext.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@
import com.linecorp.armeria.common.logging.RequestLogAccess;
import com.linecorp.armeria.common.logging.RequestLogBuilder;
import com.linecorp.armeria.common.util.SafeCloseable;
import com.linecorp.armeria.common.util.Unwrappable;
import com.linecorp.armeria.internal.common.JavaVersionSpecific;
import com.linecorp.armeria.internal.common.RequestContextUtil;
import com.linecorp.armeria.server.ServiceRequestContext;
Expand All @@ -61,7 +62,7 @@
* A server-side {@link Request} has a {@link ServiceRequestContext} and
* a client-side {@link Request} has a {@link ClientRequestContext}.
*/
public interface RequestContext {
public interface RequestContext extends Unwrappable {
jrhee17 marked this conversation as resolved.
Show resolved Hide resolved

/**
* Returns the context of the {@link Request} that is being handled in the current thread.
Expand Down Expand Up @@ -464,6 +465,11 @@ default ByteBufAllocator alloc() {
@MustBeClosed
SafeCloseable push();

@Override
default RequestContext unwrap() {
Copy link
Contributor

Choose a reason for hiding this comment

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

master branch has been merged. Let's do the same work for unwrapAll(). 😉

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's do the same work for unwrapAll()

Do you mean override the return type for unwrapAll?
I thought it's difficult to do this with the current constraints provided by Unwrappable 😅

ref: #4338 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

Couldn't we safely cast the return type to RequestContext?

@Override
default RequestContext unwrapAll() {
    return (RequestContext) Unwrappable.super.unwrapAll();
}

Copy link
Contributor Author

@jrhee17 jrhee17 Aug 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Sorry, I guess I should've made my comment on the other PR clearer 😅

To be precise, as you said we can override the return type as RequestContext for objects that are "unwrappable".

However, I'm unsure how the API should be designed for objects that wrap an Unwrappable (i.e. via AbstractUnwrappable)

  1. If AbstractUnwrappable#unwrapAll returns an Object:
    In this case AbstractUnwrappable#unwrapAll returns an Object whereas RequestContext#unwrapAll returns a RequestContext, which causes a return type collision.
    e.g.
'unwrapAll()' in 'com.linecorp.armeria.common.util.AbstractUnwrappable' clashes with 'unwrapAll()' in 'com.linecorp.armeria.common.RequestContext'; attempting to use incompatible return type
  1. If AbstractUnwrappable#unwrapAll returns T:
    I'm not sure if there's type safe way to know the innermost type of a wrappable.
final Foo foo = new Foo();
final Bar<Foo> bar = new Bar<>(foo);
final Qux<Bar<Foo>> qux = new Qux<>(bar);

I guess it's not too late to modify to a type unsafe API though 😅
e.g.

<U extends Unwrappable> U unwrapAll()

ref: #4338 (comment)

Let me know if you have a better idea though 😅

Copy link
Contributor

Choose a reason for hiding this comment

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

'unwrapAll()' in 'com.linecorp.armeria.common.util.AbstractUnwrappable' clashes with 'unwrapAll()' in 'com.linecorp.armeria.common.RequestContext'; attempting to use incompatible return type

The error seems to happen in RequestContextWrapper. How about overriding the return type of unwrapAll() in RequestContextWrapper?
I don't see that we need to change the return type of AbstractUnwrappable#unwrapAll.
The subtype of AbstractUnwrappable or Unwrappable freely overrides the return type if necessary.

public abstract class RequestContextWrapper<T extends RequestContext>
        extends AbstractUnwrappable<T> implements RequestContext {

    @Override
    public RequestContext unwrapAll() {
        return (RequestContext) super.unwrapAll();
    }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I think you're right. I guess we can override the return type for wrapper types only. Done!

return (RequestContext) Unwrappable.super.unwrap();
}

/**
* Immediately run a given {@link Runnable} with this context.
*/
Expand Down
Loading