api,core: Add ServerInterceptor2 interface #7746

ericgribkoff · 2020-12-22T08:55:57Z

ServerInterceptor2 allows rewriting the ServerMethodDefinition. This accommodates, among other things, setting stream tracers on the ServerMethodDefinition as part of an interceptor, rather than requiring stream tracer factories to be added directly to the server builder prior to construction.

Since some aspects of the stats trace context are currently created when the stream begins and before method handler lookup has occurred, which means any stream tracers attached via a ServerInterceptor2 must be invoked later, after method lookup. And one of the abilities of a server stream tracer is filtering the context in which the call will take place. This PR rewires things in ServerImpl to accomodate a (partially) delayed creation of the stats trace context itself and the context in which the call operates.

Notes for review:

The StatsTraceContext class already had several client-specific and server-specific methods. This PR adds more. It is probably worthwhile splitting the class into a ClientStatsTraceContext and ServerStatsTraceContext.
Looking at it now, the Javadoc for ServerBuilder#intercept(io.grpc.ServerInterceptor2)is confusing when discussing "order" in relation to the existing ServerInterceptor, as the ServerInterceptor2 API is actually invoked prior to the call starting, so outside of the ServerInteceptor#startCall ordering entirely - but ServerInterceptor2 can still rewrite the ServerCallHandler to mimic the operation of a ServerInterceptor.

ejona86

Sending the review comments I have. I should have sent them out last week, but forgot to. They are incomplete, and even includes a note for myself, but probably are helpful.

ejona86 · 2020-12-31T00:18:54Z

api/src/main/java/io/grpc/ServerMethodDefinition.java

@@ -24,11 +27,13 @@
 public final class ServerMethodDefinition<ReqT, RespT> {
  private final MethodDescriptor<ReqT, RespT> method;
  private final ServerCallHandler<ReqT, RespT> handler;
+  private final List<ServerStreamTracer.Factory> streamTracerFactories;


This class must not be mutable.

ejona86 · 2020-12-31T00:26:39Z

core/src/main/java/io/grpc/internal/ServerImpl.java

@@ -654,6 +685,23 @@ public void cancelled(Context context) {
    }
  }

+  /** Wrapper to convert ServerInterceptor to ServerInterceptor2. */
+  private static final class ServerInterceptorConverter implements ServerInterceptor2 {


This should be provided as a method on ServerInterceptors.

ejona86 · 2020-12-31T00:28:07Z

api/src/main/java/io/grpc/ServerBuilder.java

+  /**
+   * Adds a {@link ServerInterceptor2} that is run for all services on the server.  Interceptors
+   * added through this method always run before per-service interceptors added through {@link
+   * ServerInterceptors} and any interceptors added through {@link #intercept(ServerInterceptor)}.


I think we want to be able to mix ServerInterceptor and ServerInterceptor2 together, such that they are run in the order added, not in two stages. I imagine that would be implemented by converting ServerInterceptors to ServerInterceptor2 when being added.

That is much clearer. Done.

ejona86 · 2020-12-31T00:45:43Z

core/src/main/java/io/grpc/internal/ServerImpl.java

@@ -554,13 +558,28 @@ private void runInternal() {
              context.cancel(null);
              return;
            }
-            listener = startCall(stream, methodName, method, headers, context, statsTraceCtx, tag);
+            ServerMethodDefinition<?, ?> interceptedDef = getInterceptedMethodDef(method);


Since interceptors can now do non-trivial processing ahead-of-time for the method, I think we'll try to do this per-RPC only for the fallbackRegistry case. We can pre-apply the interceptors to the services in registry. Although there may be complications, like with reflection. Not essential today, but at least put a TODO.

ejona86 · 2020-12-31T00:56:17Z

core/src/main/java/io/grpc/internal/ServerImpl.java

-      ServerCallHandler<ReqT, RespT> handler = methodDef.getServerCallHandler();
-      for (ServerInterceptor interceptor : interceptors) {
-        handler = InternalServerInterceptors.interceptCallHandler(interceptor, handler);
+      Context previous = context.attach();


Note to self: Context here and in JumpToApplicationThreadServerStreamListener seems should see some simplification. Why is it doing this?

ejona86 · 2020-12-31T01:00:41Z

core/src/main/java/io/grpc/internal/StatsTraceContext.java

+      StreamTracer[] tracerArr = new StreamTracer[factories.size()];
+      for (int i = 0; i < tracerArr.length; i++) {
+        ServerStreamTracer tracer = factories.get(i).newServerStreamTracer(fullMethodName, headers);
+        context = tracer.filterContext(context).withCancellation();


No, no. Every CancellableContext must be canceled at some point in the future. It's not clear to me why we'd even need to involve this code with CancellabelContext.

It doesn't need to be CancellableContext here. As far as the ServerStreamTracer#filterContext it should just be a Context. This is just a result of bad 'type matching' on my part: by the time this method is invoked from ServerImpl, we already have a CancellableContext - I think it's necessary very early in the call handling process for e.g. RPC deadlines - and I was just blindly maintaining that type here.

But I'm not sure I fully understand the problem that you are describing: looking at what withCancellation actually does, it's obvious that it's heavier weight than we actually need but I understood your comment to mean this is actually incorrect due to a mishandling of the context's lifetime. However, since we begin with a CancellableContext from ServerImpl (created and managed by the existing code prior to this PR, so I don't think I've broken anything there, probably :)) and according to the CancellableContext javadoc it is "A context which inherits cancellation from its parent but which can also be independently cancelled and which will propagate cancellation to its descendants" shouldn't this actually work as-is? Since the parent CancellableContext from ServerImpl.ServerTransportListenerImpl#createContext will eventually be cancelled and that cancellation propagated to the descendants created here?

I'm worried that I'm misunderstanding something fundamental here, but the basic constraint I was operating under was that, when this method is invoked from ServerImpl.ServerTransportListenerImpl#streamCreatedInternal, we have a CancellableContext and want to get a CancellableContext back. The current code is just a ham-handed way of maintaining that type, but is there a reason that we don't want a CancellableContext at this point at all and the "conversion" from Context to CancellableContext currently done in ServerImpl.ServerTransportListenerImpl#createContext should be deferred until after these interceptor-provided tracers have been invoked?

I was talking about "To avoid leaking memory, every CancellableContext must have a defined lifetime, after which it is guaranteed to be cancelled." Yes, if the parent is cancellable you are guaranteed this one will have a lifetime, although you aren't guaranteed the lifetime is appropriate (the lifetime could be the entire process lifetime in some Context cases). I think the only reason you know the lifetime is appropriate here is because you say "statsTraceCtx.serverFilterContext()" is expected to be called first, at which point the context should already be scoped to the call.

But there's costs to cancellable contexts, and it is really weird that they don't manage their own lifetime, and it's weird they are created in a loop (instead of only once at the end). I think this code should just be dealing with normal contexts; normal context input and normal context output.

…ptor calls

ericgribkoff

Thanks for the review! I addressed most of the comments, and left a long-winded follow-up query in regards to properly handling the CancellableContext issue

ericgribkoff · 2021-01-12T13:08:53Z

core/src/main/java/io/grpc/internal/StatsTraceContext.java

+      StreamTracer[] tracerArr = new StreamTracer[factories.size()];
+      for (int i = 0; i < tracerArr.length; i++) {
+        ServerStreamTracer tracer = factories.get(i).newServerStreamTracer(fullMethodName, headers);
+        context = tracer.filterContext(context).withCancellation();


It doesn't need to be CancellableContext here. As far as the ServerStreamTracer#filterContext it should just be a Context. This is just a result of bad 'type matching' on my part: by the time this method is invoked from ServerImpl, we already have a CancellableContext - I think it's necessary very early in the call handling process for e.g. RPC deadlines - and I was just blindly maintaining that type here.

But I'm not sure I fully understand the problem that you are describing: looking at what withCancellation actually does, it's obvious that it's heavier weight than we actually need but I understood your comment to mean this is actually incorrect due to a mishandling of the context's lifetime. However, since we begin with a CancellableContext from ServerImpl (created and managed by the existing code prior to this PR, so I don't think I've broken anything there, probably :)) and according to the CancellableContext javadoc it is "A context which inherits cancellation from its parent but which can also be independently cancelled and which will propagate cancellation to its descendants" shouldn't this actually work as-is? Since the parent CancellableContext from ServerImpl.ServerTransportListenerImpl#createContext will eventually be cancelled and that cancellation propagated to the descendants created here?

I'm worried that I'm misunderstanding something fundamental here, but the basic constraint I was operating under was that, when this method is invoked from ServerImpl.ServerTransportListenerImpl#streamCreatedInternal, we have a CancellableContext and want to get a CancellableContext back. The current code is just a ham-handed way of maintaining that type, but is there a reason that we don't want a CancellableContext at this point at all and the "conversion" from Context to CancellableContext currently done in ServerImpl.ServerTransportListenerImpl#createContext should be deferred until after these interceptor-provided tracers have been invoked?

ericgribkoff · 2021-01-12T13:22:03Z

core/src/main/java/io/grpc/internal/ServerImpl.java

@@ -554,13 +558,28 @@ private void runInternal() {
              context.cancel(null);
              return;
            }
-            listener = startCall(stream, methodName, method, headers, context, statsTraceCtx, tag);
+            ServerMethodDefinition<?, ?> interceptedDef = getInterceptedMethodDef(method);


ericgribkoff · 2021-01-12T13:23:10Z

api/src/main/java/io/grpc/ServerBuilder.java

+  /**
+   * Adds a {@link ServerInterceptor2} that is run for all services on the server.  Interceptors
+   * added through this method always run before per-service interceptors added through {@link
+   * ServerInterceptors} and any interceptors added through {@link #intercept(ServerInterceptor)}.


That is much clearer. Done.

ericgribkoff · 2021-01-12T13:23:16Z

api/src/main/java/io/grpc/ServerMethodDefinition.java

@@ -24,11 +27,13 @@
 public final class ServerMethodDefinition<ReqT, RespT> {
  private final MethodDescriptor<ReqT, RespT> method;
  private final ServerCallHandler<ReqT, RespT> handler;
+  private final List<ServerStreamTracer.Factory> streamTracerFactories;


ericgribkoff · 2021-01-12T13:23:36Z

core/src/main/java/io/grpc/internal/ServerImpl.java

@@ -654,6 +685,23 @@ public void cancelled(Context context) {
    }
  }

+  /** Wrapper to convert ServerInterceptor to ServerInterceptor2. */
+  private static final class ServerInterceptorConverter implements ServerInterceptor2 {


ericgribkoff · 2021-01-12T13:37:08Z

Also, I know we discussed a better name than ServerInterceptor2 - I didn't want to push a rename commit mid-review, but ServerMethodDefinitionInterceptor is definitely more informative (it's a bit long, but I'm not sure if that's really a concern - it's 2 characters shorter than our current io.grpc.api co-champions,PartialForwardingClientCallListener and PartialForwardingServerCallListener :))

ejona86

Oh, apparently I've have some pending comments sitting here for a long time. I'm really surprised github didn't GC them.

ejona86 · 2021-01-26T21:19:18Z

api/src/main/java/io/grpc/ServerMethodDefinition.java

    this.method = method;
    this.handler = handler;
+    this.tracerFactories = Collections.unmodifiableList(tracerFactories);


This is not immutable. You need to make a copy.

List<ServerStreamTracer.Factory> l = new ArrayList<>(); def = def.withStreamTracerFactories(l); l.add(foo);

ejona86 · 2021-01-26T21:23:19Z

api/src/main/java/io/grpc/ServerMethodDefinition.java

+   */
+  @ExperimentalApi("https://github.com/grpc/grpc-java/issues/????")
+  public ServerMethodDefinition<ReqT, RespT> withStreamTracerFactories(
+      List<ServerStreamTracer.Factory> tracerFactories) {


This is an annoying API to use, as it requires getting the current list, copying it, and adding your own factory to the list, all before calling this API. How about a withAdditionalStreamTracerFactory(ServerStreamTracer.Factory) which adds to the list for you. (withStreamTracerFactory may be an okay name, but that's more for an API review meeting discussion.)

ejona86 · 2021-01-26T21:26:06Z

api/src/main/java/io/grpc/ServerMethodDefinition.java

+   * @since 1.36.0
+   */
+  @ExperimentalApi("https://github.com/grpc/grpc-java/issues/????")
+  public List<ServerStreamTracer.Factory> getStreamTracerFactories() {


There may be value in this being a Collection. List is fine for now, but it should be discussed in the API review meeting.

api,core: Add ServerInterceptor2 interface

7b1f160

ericgribkoff requested a review from ejona86 December 22, 2020 08:55

ejona86 reviewed Jan 6, 2021

View reviewed changes

ericgribkoff added 3 commits January 8, 2021 20:41

keep ServerMethodDefinition immutable

48148e4

add utility method for ServerInterceptorConverter, interleave interce…

0c07200

…ptor calls

add TODO

c7ac3d1

ericgribkoff commented Jan 12, 2021

View reviewed changes

ericgribkoff mentioned this pull request Jan 26, 2021

Add metrics and tracing support for Java GoogleCloudPlatform/traffic-director-grpc-examples#13

Merged

ejona86 mentioned this pull request Feb 18, 2021

Add the ability to have a separate thread pool/executor for each endpoint #7874

Closed

ejona86 reviewed Jun 8, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

api,core: Add ServerInterceptor2 interface #7746

api,core: Add ServerInterceptor2 interface #7746

ericgribkoff commented Dec 22, 2020

ejona86 left a comment

ejona86 Dec 31, 2020

ericgribkoff Jan 12, 2021

ejona86 Dec 31, 2020

ericgribkoff Jan 12, 2021

ejona86 Dec 31, 2020

ericgribkoff Jan 12, 2021

ejona86 Dec 31, 2020

ericgribkoff Jan 12, 2021

ejona86 Dec 31, 2020

ejona86 Dec 31, 2020

ericgribkoff Jan 12, 2021

ejona86 Jan 26, 2021

ericgribkoff left a comment

ericgribkoff Jan 12, 2021

ericgribkoff Jan 12, 2021

ericgribkoff Jan 12, 2021

ericgribkoff Jan 12, 2021

ericgribkoff Jan 12, 2021

ericgribkoff commented Jan 12, 2021

ejona86 left a comment

ejona86 Jan 26, 2021

ejona86 Jan 26, 2021

ejona86 Jan 26, 2021

api,core: Add ServerInterceptor2 interface #7746

Are you sure you want to change the base?

api,core: Add ServerInterceptor2 interface #7746

Conversation

ericgribkoff commented Dec 22, 2020

ejona86 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericgribkoff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericgribkoff commented Jan 12, 2021

ejona86 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment