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 ServerInterceptor throwing exceptions are not catched #28053

Closed
slinkydeveloper opened this issue Sep 19, 2022 · 15 comments · Fixed by #28063
Closed

gRPC ServerInterceptor throwing exceptions are not catched #28053

slinkydeveloper opened this issue Sep 19, 2022 · 15 comments · Fixed by #28063
Labels
area/grpc gRPC kind/bug Something isn't working
Milestone

Comments

@slinkydeveloper
Copy link

Describe the bug

Hi all, I'm trying to use an interceptor coming from an external package, which throws an exception when executing interceptCall. When this happens, quarkus will print the exception as uncaught but won't reply back.

Looking at the docs of ServerInterceptor, throwing an exception there looks like a valid implementation:

If the implementation throws an exception, call will be closed with an error.

Expected behavior

Response back with error grpc status code INTERNAL and some status message indicating the failure (or maybe just "Internal server error")

Actual behavior

No response back

How to Reproduce?

  1. Create an interceptor throwing an exception
  2. Try to send a request using grpcurl, e.g. grpcurl -vv -plaintext -d '{"name":"Francesco"}' localhost:9000 hello.HelloGrpc/SayHello

See the reproducer attached

reproducer.zip

Output of uname -a or ver

No response

Output of java -version

openjdk version "17.0.4.1" 2022-08-12

GraalVM version (if different from Java)

No response

Quarkus version or git rev

2.12.2.Final

Build tool (ie. output of mvnw --version or gradlew --version)

No response

Additional information

No response

@slinkydeveloper slinkydeveloper added the kind/bug Something isn't working label Sep 19, 2022
@quarkus-bot quarkus-bot bot added the area/grpc gRPC label Sep 19, 2022
@quarkus-bot
Copy link

quarkus-bot bot commented Sep 19, 2022

/cc @alesj, @cescoffier

@alesj
Copy link
Contributor

alesj commented Sep 19, 2022

New Vert.x gRPC impl already responds with 500:

@cescoffier I guess this could be fixed as part of current work?
(but I would probably need some hint on where / how to fix it)

@alesj
Copy link
Contributor

alesj commented Sep 19, 2022

Currently you get this (on my WIP branch ...):

@alesj
Copy link
Contributor

alesj commented Sep 19, 2022

@cescoffier
Copy link
Member

@alesj we may want to fix in the current server too (if it does not require breaking something), as I would qualify this as a bug (and thanks for the report @slinkydeveloper !)

@cescoffier
Copy link
Member

The question is does the current server let us catch these exceptions?

@alesj
Copy link
Contributor

alesj commented Sep 19, 2022

The question is does the current server let us catch these exceptions?

See VertxCoreRecorder ...

        vertx.exceptionHandler(new Handler<Throwable>() {
            @Override
            public void handle(Throwable error) {
                LOGGER.error("Uncaught exception received by Vert.x", error);
            }
        });

@alesj
Copy link
Contributor

alesj commented Sep 19, 2022

And it looks like we would have to have 2 diff solutions / fixes:

Unless we leave the new gRPC with already provided 500 ...

@cescoffier
Copy link
Member

@alesj the Vert.x exceptionHandler is not going to be able to close the gRPC connection. I'm actually surprised that grpc-java does not handle that automatically.

@cescoffier
Copy link
Member

Yes, it does not do anything, so it bubbles up.

@slinkydeveloper
Copy link
Author

slinkydeveloper commented Sep 19, 2022

I'm actually surprised that grpc-java does not handle that automatically.

The regular grpc server handles this case AFAIK. You should be able to see it somewhere in ServerImpl

@cescoffier
Copy link
Member

ah yes, in runInternal. So, why does our server not doing this? Might be a gap in Vert.x

@alesj
Copy link
Contributor

alesj commented Sep 19, 2022

          try {
            listener = startWrappedCall(methodName, Futures.getDone(future), headers);
          } catch (Throwable ex) {
            stream.close(Status.fromThrowable(ex), new Metadata());
            context.cancel(null);
            throw new IllegalStateException(ex);

The question is why there is no response ...
Where I guess Vert.x exceptionHandler catches this ISE.

@cescoffier
Copy link
Member

Ok, I may be on something...

@cescoffier
Copy link
Member

So... it was my fault. Opened #28063.

cescoffier added a commit to cescoffier/quarkus that referenced this issue Sep 21, 2022
@quarkus-bot quarkus-bot bot added this to the 2.14 - main milestone Sep 26, 2022
@gsmet gsmet modified the milestones: 2.14 - main, 2.13.1.Final Sep 30, 2022
gsmet pushed a commit to gsmet/quarkus that referenced this issue Oct 3, 2022
igorregis pushed a commit to igorregis/quarkus that referenced this issue Oct 16, 2022
igorregis pushed a commit to igorregis/quarkus that referenced this issue Oct 16, 2022
igorregis pushed a commit to igorregis/quarkus that referenced this issue Oct 16, 2022
igorregis pushed a commit to igorregis/quarkus that referenced this issue Oct 17, 2022
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
Development

Successfully merging a pull request may close this issue.

4 participants