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

Lost context in @Blocking gRPC Service behind a GlobalInterceptor #26830

Closed
spencercjh opened this issue Jul 20, 2022 · 5 comments · Fixed by #29028, #30420 or #37225
Closed

Lost context in @Blocking gRPC Service behind a GlobalInterceptor #26830

spencercjh opened this issue Jul 20, 2022 · 5 comments · Fixed by #29028, #30420 or #37225
Labels

Comments

@spencercjh
Copy link
Contributor

I found that Quarkus Server register io.quarkus.grpc.runtime.supports.blocking.BlockingServerInterceptor for each blocking gRPC service with the @Blocking annotation before it starts.

If some global interceptors (with @GlobalInterceptor) set the Context value will not work. The Context will still be empty in the actual rpc method handler.

Is it a normal phenomenon because Context should be placed after BlockingServerInterceptor 's vert.x.executeBlocking? There are some associated issues: #14665, #13959.

Finally, I replace the @GlobalInterceptor with @RegisterInterceptor(MyInterceptor.class), and everything is to be ok which seems to prove my point.

I don't particularly understand the technical details of it, and these are what I got from trying and debugging. I've been only using Spring and Micronaut before.

origin question:

https://stackoverflow.com/questions/73039706/lost-context-in-blocking-grpc-service-with-quarkus-behind-a-globalinterceptor

@quarkus-bot quarkus-bot bot added the area/grpc gRPC label Jul 20, 2022
@quarkus-bot
Copy link

quarkus-bot bot commented Jul 20, 2022

/cc @cescoffier, @michalszynkiewicz

@spencercjh
Copy link
Contributor Author

If my description is correct, I would like to add the above to the relevant docs(gRPC interceptor, @Blocking) with a PR.

@cescoffier
Copy link
Member

@spencercjh sorry for the delay.
By any chance, do you have a reproducer?

@cescoffier cescoffier added the triage/needs-reproducer We are waiting for a reproducer. label Sep 12, 2022
@spencercjh spencercjh changed the title [GRPC NEED MORE Docs] Lost context in @Blocking gRPC Service behind a GlobalInterceptor Lost context in @Blocking gRPC Service behind a GlobalInterceptor Sep 15, 2022
@cescoffier
Copy link
Member

I was able to reproduce it.

@cescoffier cescoffier removed the triage/needs-reproducer We are waiting for a reproducer. label Sep 26, 2022
cescoffier added a commit to cescoffier/quarkus that referenced this issue Sep 26, 2022
The problem comes from the default gRPC context storage using a thread-local.
This commit overrides the storage implementation (using the recommended method) to use the duplicated context and fallback to a thread-local.
cescoffier added a commit to cescoffier/quarkus that referenced this issue Sep 26, 2022
The problem comes from the default gRPC context storage using a thread-local.
This commit overrides the storage implementation (using the recommended method) to use the duplicated context and fallback to a thread-local.
@spencercjh
Copy link
Contributor Author

spencercjh commented Sep 27, 2022

@cescoffier sorry for the delay...

                                                                                                                                                        
                                                                                                                                                             
                                                                                                                                                             
                                                                                                                                                             
                                     Global gRPC interceptor      vert.x BlockingServerInterceptor   Actual request handler                                  
                                             +----+                           +----+                         +----+                                          
                                             |    |                           |    |                         |    |                                          
                                  request    |    |                           |    |                         |    |                                          
                        -------------------->|    | ------------------------->|    |-----------------------> |    |---------+                                
                                             |    |                           |    |                         |    |         |                                
                                             |    |                           |    |                         |    |         | process                        
                                             |    |                           |    |                         |    |         |                                
                                             |    |                           |    |                         |    |         v                                
                                             |    |                        -->|    | <---------------------- |    |<---------                                
                                             +----+                    ---/   +----+    throw exception      +----+                                          
                                                      hang up here  --/                                                                                      
                                                      and no response to stub becasue  onError isn't invoked.                                                
                                                                                                                       

The best practice for gRPC is to use the Global gRPC Interceptor to handle exceptions as a last resort, but due to the problems mentioned in this issue, this operation is not feasible (under @Blocking gRPC), the thread will hang, the request will not be responded to correctly, and the code will not run to the interceptor as expected.

I actually wanted to write the gRPC service with the asynchronous API that Quarkus recommends more, but the gRPC and Protobuf we use are managed by Buf, not just pure protoc, so we can't generate a whole new set of gRPC code like in the documentation.

Maybe we need another issue to discuss the global interceptor order under @Blocking.

gsmet pushed a commit to gsmet/quarkus that referenced this issue Oct 3, 2022
The problem comes from the default gRPC context storage using a thread-local.
This commit overrides the storage implementation (using the recommended method) to use the duplicated context and fallback to a thread-local.

(cherry picked from commit b82b235)
igorregis pushed a commit to igorregis/quarkus that referenced this issue Oct 16, 2022
The problem comes from the default gRPC context storage using a thread-local.
This commit overrides the storage implementation (using the recommended method) to use the duplicated context and fallback to a thread-local.
igorregis pushed a commit to igorregis/quarkus that referenced this issue Oct 16, 2022
The problem comes from the default gRPC context storage using a thread-local.
This commit overrides the storage implementation (using the recommended method) to use the duplicated context and fallback to a thread-local.
igorregis pushed a commit to igorregis/quarkus that referenced this issue Oct 16, 2022
The problem comes from the default gRPC context storage using a thread-local.
This commit overrides the storage implementation (using the recommended method) to use the duplicated context and fallback to a thread-local.
igorregis pushed a commit to igorregis/quarkus that referenced this issue Oct 17, 2022
The problem comes from the default gRPC context storage using a thread-local.
This commit overrides the storage implementation (using the recommended method) to use the duplicated context and fallback to a thread-local.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment