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.

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

gRPC Context lost when using @Blocking #14665

Closed
gaetancollaud opened this issue Jan 28, 2021 · 11 comments · Fixed by #14697
Closed

gRPC Context lost when using @Blocking #14665

gaetancollaud opened this issue Jan 28, 2021 · 11 comments · Fixed by #14697
Labels
area/grpc gRPC kind/bug Something isn't working
Milestone

Comments

@gaetancollaud
Copy link
Contributor

Describe the bug
I use a grpc service with some interceptors. One of the goal of those interceptors is to forward the JWT token from the gRPC service to any client calls later in the code. For this is use the gRPC context. This works fine if I don't use the @Blocking annotation.

As expected the @Blocking annotation makes my code run into a worker thread. But since I have other grpc calls in this code the JWT is null since the context was lost.

I tried @ActivateRequestContext on my method but it seems that it's not compatible with @Blocking (see #13358).

This issue sounds similar to #13959 but there is no mention of blocking. And for me the context set in the interceptor works so I don't think if it's related.

Expected behavior

I can use the gRPC context in blocking method.

Actual behavior
Context is lost when using @Blocking

To Reproduce

If it make sense I can try to create a small reproducer, but I cannot share my current code. Let me know.

Steps to reproduce the behavior:

  1. Have a grpc service
  2. Have a grpc interceptor that change the gRPC Context
  3. Annotation a gRPC method with @Blocking
  4. Try to use this context in your blocking method.

Configuration
irrelevant

Screenshots
irrelevant

Environment (please complete the following information):

  • Output of uname -a or ver: Linux gaetan-desktop 5.4.0-64-generic #72-Ubuntu SMP Fri Jan 15 10:27:54 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux
  • Output of java -version:
openjdk version "11.0.10" 2021-01-19
OpenJDK Runtime Environment GraalVM CE 21.0.0 (build 11.0.10+8-jvmci-21.0-b06)
OpenJDK 64-Bit Server VM GraalVM CE 21.0.0 (build 11.0.10+8-jvmci-21.0-b06, mixed mode, sharing)
  • Quarkus version or git rev: 1.11.0.Final
  • Build tool (ie. output of mvnw --version or gradlew --version):
Apache Maven 3.6.3
Maven home: /usr/share/maven
Java version: 11.0.10, vendor: GraalVM Community, runtime: /home/gaetan/.sdkman/candidates/java/21.0.0.r11-grl
Default locale: en_US, platform encoding: UTF-8
OS name: "linux", version: "5.4.0-64-generic", arch: "amd64", family: "unix"

Additional context
none

@gaetancollaud gaetancollaud added the kind/bug Something isn't working label Jan 28, 2021
@ghost ghost added the area/grpc gRPC label Jan 28, 2021
@ghost
Copy link

ghost commented Jan 28, 2021

/cc @cescoffier, @michalszynkiewicz

@gaetancollaud
Copy link
Contributor Author

gaetancollaud commented Jan 28, 2021

My dirty workaround is this:

@Singleton
public class GrpcSchemaService extends SchemaServiceGrpc.SchemaServiceImplBase {

  private final ManagedExecutor managedExecutor;

  @Override
  public void saveSchema(SaveSchemaRequest request, StreamObserver<SaveSchemaResponse> responseObserver) {

    // saving the request context locally
    final Context current = Context.current();
    
    // running in a worker thread
    managedExecutor.execute(
        // wrapping runnable in inital context
        current.wrap(() -> {
              // here my jwt is present since I wrapped the runable in the initial context
              String jwt = AuthHeaderInterceptor.USER_IDENTITY.get().getAuthorizationHeader()

              responseObserver.onNext(...);
              responseObserver.onCompleted();
            }));
  }

Basically I manually use a managedExecutor and I forward the gRPC context to the thread. But the issue is that I have to do this for all the methods. 😞

The issue now is that my GrpcErrorInterceptor is bypassed 😭

@cescoffier
Copy link
Member

Why didn't you use an interceptor directly? Wouldn't that work?

@gaetancollaud
Copy link
Contributor Author

gaetancollaud commented Jan 28, 2021

That's the point, I'm using interceptors in my spring-boot project. And I would like to use them with Quarkus too. The issue is that the context is lost between the service interceptor (that reads and store the JWT) and the client interceptor (that add the JWT in the headers). When using @Blocking, the service interceptor is running in the IO thread and client interceptor is running in the worker thread. So the gRPC context is lost.

I need worker thread because I have blocking calls (other than grpc).

So in my opinion, @Blocking should forward the gRPC context somehow. What do you think ?

@cescoffier
Copy link
Member

cescoffier commented Jan 28, 2021

Yes, but that's the other issue you referenced. We need to implement the Context Propagation SPI for gRPC. I didn't have the time so far.

@gaetancollaud
Copy link
Contributor Author

Ok, so it's not a bug, it's just a missing feature.

Is this something you planned ? Is there any way I can help ?

If you have already an implementation idea in mind, just tell it and I may be able to implement it. I can come on zulip if it's more convenient for you.

@gaetancollaud
Copy link
Contributor Author

I found this: https://github.com/quarkusio/quarkus/blob/master/extensions/grpc/runtime/src/main/java/io/quarkus/grpc/runtime/supports/BlockingServerInterceptor.java#L64-L73

So if I save the context before the vertx.executeBlocking and wrap it inside it should work, what do you think ? Is there more than that ? I would be happy to come up with a PR if you think it's the right direction.

@cescoffier
Copy link
Member

Yes, I would start with what you mentioned, however, you would need to restore the original context too.

I'm also wondering if we need an implementation of ThreadContextProvider that would do that for us.

@gaetancollaud
Copy link
Contributor Author

gaetancollaud commented Jan 28, 2021

Yes, I would start with what you mentioned, however, you would need to restore the original context too.

Context.wrap( myRunnable) should in theory restore the context automatically.

I'm also wondering if we need an implementation of ThreadContextProvider that would do that for us.

Can you guide me for this one ?

  • I create a class that implement ThreadContextProvider
  • I reference it in /runtime/src/main/resources/META-INF/services/org.eclipse.microprofile.context.spi.ThreadContextProvider
  • Something else ?

With this solution I won't be able to use wrap I will have to use attach and restore the previous context (not a big deal).

@cescoffier
Copy link
Member

Yes, I believe that's the only thing required.

@gaetancollaud
Copy link
Contributor Author

gaetancollaud commented Jan 29, 2021

It turned out it was not only that. The ThreadContextProvider is not magically picked up. 😞

So I created two PRs (see above). One with the ThreadContextProvider (#14687) wrapper and one without (#14697).

@cescoffier Let me know which one you think it's best and if I need to improve something. I will let #14687 in draft since I don't think I used the ThreadContextListener correctly. I saw that the resteasy implementation is rather complex. See SmallRyeContextPropagationProcessor But maybe it's the way to go

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/grpc gRPC kind/bug Something isn't working
Projects
None yet
3 participants