From ed022b547f5c6d9d3e4e74a123ae804cbbdf6572 Mon Sep 17 00:00:00 2001 From: ST-DDT Date: Sat, 3 Oct 2020 09:10:49 +0200 Subject: [PATCH 1/2] Add implementation note regarding server interceptors and thread locals --- api/src/main/java/io/grpc/ServerInterceptor.java | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/api/src/main/java/io/grpc/ServerInterceptor.java b/api/src/main/java/io/grpc/ServerInterceptor.java index 6a4a1e58fdc..eaf53c573d5 100644 --- a/api/src/main/java/io/grpc/ServerInterceptor.java +++ b/api/src/main/java/io/grpc/ServerInterceptor.java @@ -29,6 +29,12 @@ *
  • Logging and monitoring call behavior
    • *
  • Delegating calls to other servers
    • * + * + *

    Implementation note: Implementations that provide thread local variables must remove + * them before they return from {@link #interceptCall(ServerCall, Metadata, ServerCallHandler)} + * because each message that is processed as part of the call might be handled by a different + * thread (including the first message). If you wish to provide the thread local for the duration + * of the entire call you have to reassign and clear them in each method of the returned listener. */ @ThreadSafe public interface ServerInterceptor { From cd320241f6a09ed08fb7d7a809a37d2ef464fccf Mon Sep 17 00:00:00 2001 From: Daniel Theuke Date: Tue, 13 Oct 2020 16:30:00 +0200 Subject: [PATCH 2/2] Move thread hostility note to *Call.Listener --- api/src/main/java/io/grpc/ClientCall.java | 5 ++++- api/src/main/java/io/grpc/ClientInterceptor.java | 5 +++++ api/src/main/java/io/grpc/ServerCall.java | 5 ++++- api/src/main/java/io/grpc/ServerInterceptor.java | 9 ++++----- 4 files changed, 17 insertions(+), 7 deletions(-) diff --git a/api/src/main/java/io/grpc/ClientCall.java b/api/src/main/java/io/grpc/ClientCall.java index b66924d4102..ac32dccda56 100644 --- a/api/src/main/java/io/grpc/ClientCall.java +++ b/api/src/main/java/io/grpc/ClientCall.java @@ -104,7 +104,10 @@ public abstract class ClientCall { * Callbacks for receiving metadata, response messages and completion status from the server. * *

    Implementations are free to block for extended periods of time. Implementations are not - * required to be thread-safe. + * required to be thread-safe, but they must not be thread-hostile. The caller is free to call + * an instance from multiple threads, but only one call simultaneously. A single thread may + * interleave calls to multiple instances, so implementations using ThreadLocals must be careful + * to avoid leaking inappropriate state (e.g., clearing the ThreadLocal before returning). */ public abstract static class Listener { diff --git a/api/src/main/java/io/grpc/ClientInterceptor.java b/api/src/main/java/io/grpc/ClientInterceptor.java index 6cb0c2420f4..c27c31c8474 100644 --- a/api/src/main/java/io/grpc/ClientInterceptor.java +++ b/api/src/main/java/io/grpc/ClientInterceptor.java @@ -32,6 +32,11 @@ *

    Providing authentication credentials is better served by {@link * CallCredentials}. But a {@code ClientInterceptor} could set the {@code * CallCredentials} within the {@link CallOptions}. + * + *

    The interceptor may be called for multiple {@link ClientCall calls} by one or more threads + * without completing the previous ones first. Refer to the + * {@link io.grpc.ClientCall.Listener ClientCall.Listener} docs for more details regarding thread + * safety of the returned listener. */ @ThreadSafe public interface ClientInterceptor { diff --git a/api/src/main/java/io/grpc/ServerCall.java b/api/src/main/java/io/grpc/ServerCall.java index 15ac0efdc93..93d76dca07d 100644 --- a/api/src/main/java/io/grpc/ServerCall.java +++ b/api/src/main/java/io/grpc/ServerCall.java @@ -46,7 +46,10 @@ public abstract class ServerCall { * close, which is guaranteed before completion. * *

    Implementations are free to block for extended periods of time. Implementations are not - * required to be thread-safe. + * required to be thread-safe, but they must not be thread-hostile. The caller is free to call + * an instance from multiple threads, but only one call simultaneously. A single thread may + * interleave calls to multiple instances, so implementations using ThreadLocals must be careful + * to avoid leaking inappropriate state (e.g., clearing the ThreadLocal before returning). */ // TODO(ejona86): We need to decide what to do in the case of server closing with non-cancellation // before client half closes. It may be that we treat such a case as an error. If we permit such diff --git a/api/src/main/java/io/grpc/ServerInterceptor.java b/api/src/main/java/io/grpc/ServerInterceptor.java index eaf53c573d5..272b10636cd 100644 --- a/api/src/main/java/io/grpc/ServerInterceptor.java +++ b/api/src/main/java/io/grpc/ServerInterceptor.java @@ -30,11 +30,10 @@ *

  • Delegating calls to other servers
    • * * - *

    Implementation note: Implementations that provide thread local variables must remove - * them before they return from {@link #interceptCall(ServerCall, Metadata, ServerCallHandler)} - * because each message that is processed as part of the call might be handled by a different - * thread (including the first message). If you wish to provide the thread local for the duration - * of the entire call you have to reassign and clear them in each method of the returned listener. + *

    The interceptor may be called for multiple {@link ServerCall calls} by one or more threads + * without completing the previous ones first. Refer to the + * {@link io.grpc.ServerCall.Listener ServerCall.Listener} docs for more details regarding thread + * safety of the returned listener. */ @ThreadSafe public interface ServerInterceptor {