Uncaught exceptions in server

[tonicsoft]

Please answer these questions before submitting your issue.

What version of gRPC are you using?

1.23.0

What did you expect to see?

Error response after uncaught exception

Refer to the following file/line number: https://github.com/grpc/grpc-java/blob/ba0fd84d79d66f46b4492043c62ecb0fcf85ead2/core/src/main/java/io/grpc/internal/ServerImpl.java#L558

You can see that Exception types that are not subclasses of Error or RuntimeException (known as "checked exceptions") are not caught here.

Uncaught exceptions result in no response being sent to the client (but the connection remains open) and thus, from the clients perspective, the RPC call hangs indefinitely.

All exceptions should be caught here, not just runtime exceptions and errors.

"Checked" exceptions can be thrown from code that does not declare it in the signature by several means. One example is here: https://stackoverflow.com/questions/15496/hidden-features-of-java/2131355#2131355

Another example is from any kotlin code that calls a java function that throws a checked exception.

NOTE: it looks like the same problem exists in multiple places in this file. The line number I linked is the specific one that I hit in the wild.

[dapengzhang0]

There's duplicate code in catch (RuntimeException e) block and catch (Error e) block which can be cleaned up by replacing them with catch (Throwable t). However, that would be only for the clean up purpose, not for handling hacks like "sneakyThrow" or abused usage of Error, we are not responsible to handle them.

[anarsultanov]

@dapengzhang0 replacing it with catch (Throwable t) will also include catching Exception. Do you know the reason why was it excluded before, and if this would not introduce any issue? Because for clean up purpose we can also change it to catch (RuntimeException | Error e).

[tonicsoft]

The "sneakyThrow" was a contrived example, but it demonstrates the point that the JVM cannot prevent checked exceptions from being thrown from GRPC service endpoint implementations.

Therefore it is indeed the responsibility of the GRPC thread pool/dispatcher to catch these exceptions. Otherwise, clients would be forced to either surround every implementation with a try...catch (which the IDE/compiler would likely incorrectly warn as being unnecessary) or implement a ServerInterceptor that catches the checked exceptions. Either way, this is unnecessary boiler plate code that should live in ServerImpl.

In my opinion this is a serious bug that leads to the server failing to respond to requests in some cases, based on the incorrect assumption that functions with no exception signature can never throw checked exceptions.

[dapengzhang0]

@tonicsoft

it demonstrates the point that the JVM cannot prevent checked exceptions from being thrown from GRPC service endpoint implementations.

JVM cannot prevent a method with List<String> argument be invoked with a List<Integer> argument. Is that the method's bug if it does not check the type of client's input argument?

In my opinion this is a serious bug that leads to the server failing to respond to requests in some cases, based on the incorrect assumption that functions with no exception signature can never throw checked exceptions.

If an API has sneakyThrow, it should be explicitly documented or annotated, otherwise it is the sneaky API's bug. If a user of a well-documented sneaky API does not handle the sneak throw, it is the user's bug. In any case, it is not a bug of grpc library because we did not use any sneaky API, it's the responsibility of the user who uses any sneaky API.

[ejona86]

We've not been able to use (RuntimeException | Error e) syntax because of some of the platforms we support. I am very surprised to see throw throwable does not cause javac to complain. That used to complain. Maybe since we no longer compile with Java 7 it doesn't fail compilation any more, and then Java uses type inferencing. I'd want to research it a bit more before we convert code to using it.

I don't care one bit about people doing things like EvilThrower in the SO post. I'm aware of that, because Netty does it some and it causes nothing but problems. It's equally possible using sun.misc.Unsafe. Users doing those hacks should have to deal with any fallout themselves.

I could more easily accept that someone extended Throwable separate from Exception/Error. However, I've not found a single case where that is appropriate. So it is more of a theoretical discussion and so not really useful.

The Kotlin case is more serious. It seems like another case where Kotlin breaks Java code. We still don't have much Kotlin expertise for things like this. That means it is impossible for us to avoid regressions with Kotlin code, even if it is working today.

[tonicsoft]

The sneakyThrow is just an example - perhaps it would be more interesting to discuss the use case of other JVM languages such as Kotlin. What is your opinion on the increasing number of non-java projects and libraries out there, and how they should interact with GRPC?

In addition, is there a downside to catching the checked exceptions anyway?

[tonicsoft]

Sorry, race condition. I see you already moved the discussion towards the Kotlin case :+1:

[ejona86]

I'd like a quick clarification: was this a problem you actually experienced, or were you just reading the code and noticed the problem?

2668 is when this was first discussed, but it was completely theoretical. Although later I ended up creating #4864 for Kotlin; I have no memory of that :).

[tonicsoft]

This is a problem I have actually experienced. I mentioned the line number of the case I hit in the issue description, then I noticed the same pattern elsewhere by reading the code. Therefore I'm not sure if all of the cases can be real-world problems, or just the line number I quoted.

I am currently implementing a very simple service (2-tier app, simple GRPC CRUD API backed by postgres database) in Kotlin, and I hit this issue within the first two days of development. Currently, I have opted to work around it with a ServerInterceptor:

class GlobalGrpcExceptionHandler : ServerInterceptor {
    override fun <ReqT : Any?, RespT : Any?> interceptCall(call: ServerCall<ReqT, RespT>?, headers: Metadata?, next: ServerCallHandler<ReqT, RespT>?): ServerCall.Listener<ReqT> {
        val delegate = next?.startCall(call, headers)
        return object : ForwardingServerCallListener.SimpleForwardingServerCallListener<ReqT>(delegate) {
            override fun onHalfClose() {
                try {
                    super.onHalfClose()
                } catch (e: Exception) {
                    call?.close(Status.INTERNAL
                            .withCause(e)
                            .withDescription("error message"), Metadata())
                }

            }
        }
    }

}

The app is pure Kotlin (so my Kotlin implementation class is extending the generated GrpcImplBase java class) but the postgres client is naturally a third party java library which can throw checked exceptions. One of these is what bubbled up into the Grpc handler.

[ejona86]

In addition, is there a downside to catching the checked exceptions anyway?

Previously it made the code ugly, because you couldn't easily re-throw after catching. If the type inferencing is working, then it wouldn't be as big of a deal. In general, I am still bothered with catching Throwable because we have no knowledge of what it implies. We do distinguish between RuntimeException and Error (although you'll only see a few of these cases, because in general we have a "don't throw" policy) when we are considering the code.

The bigger issue is "are we going to support this." If I am cleaning up some code and end up removing a catch Throwable, is that a bug? If I write some new code, do I need to catch Throwable? For Java we know the rules and can determine whether that is okay or not. If we add in the code for these cases, I think we would actually want to support it and be thinking about it as the code evolves.

[dapengzhang0]

https://kotlinlang.org/docs/reference/java-to-kotlin-interop.html#checked-exceptions Best practice for Kotlin API authors is to add @Throws if that API throws checked exceptions (from Kotlin calling Java), so that java users of that Kotlin API would have an easier life. I'm not sure how well Kotlin authors are following this rule, or whether Intellij or compiler/errorprone are enforcing this rule.

[ejona86]

@tonicsoft, thanks, that's helpful to know you were burned by this.

It's not generally safe to call close() within that catch, because call is not thread-safe. In your app it may be fine, though. In general for dealing with checked exceptions, I would suggest wrapping the exception in another exception (typically new RuntimeException(e)) and throwing the unchecked exception.

[tonicsoft]

@ejona86

because you couldn't easily re-throw after catching Ah makes sense.

Agreed that the question of whether to support this is important. Luckily the specific case that I hit should be easy to write a test for, and therefore I don't think it would be difficult to maintain awareness of the issue as the code evolves.

Such a test simply needs to throw a checked exception or Throwable from any GRPC synchronous service implementation.

It get's more complicated if you start looking at all the other places in the code, because we haven't verified that those code paths can definitely be reached, and how.

If it was up to me, I would add a test that reproduces the issue, and then make the minimum number of changes required to fix the issue, rather than refactor the entire class. There are a few additional tests that could be added such as doing the same thing from a streaming call, bidi call etc.

@dapengzhang0 I'm not an experienced Kotlin developer so you can take this with a pinch of salt, but I would say the @Throws annotation is only helpful for those writing Kotlin shared libraries that they expect to be used in java projects. In this case (where I'm writing a Kotlin application) I don't think the @Throws annotation helps. I tried 2 methods, neither of which worked:

1. Adding the @Throws annotation to the function that calls the java postgres client does not change anything, because there a still several layers of Kotlin code before the Grpc implementation class, so knowledge of the @Throws doesn't make it all the way up the stack
2. Adding @Throws to the grpc service implementation, which I guess is closer to the "correct" way because this is the code that is called by java code, doesn't work either. I found this a little surprising: I would have expected the generated grpc java code to no longer compile. But, it compiles OK, possibly because it's compiled with the Kotlin compiler as part of the same scope.

@ejona86 Thanks a lot for the tip. That's what happens when you copy code from SO...

[tonicsoft]

@ejona86 here's the new workaround, which has similar behaviour and is hopefully now safe:

class GlobalGrpcExceptionHandler : ServerInterceptor {
    override fun <ReqT : Any?, RespT : Any?> interceptCall(call: ServerCall<ReqT, RespT>?, headers: Metadata?, next: ServerCallHandler<ReqT, RespT>?): ServerCall.Listener<ReqT> {
        val delegate = next?.startCall(call, headers)
        return object : ForwardingServerCallListener.SimpleForwardingServerCallListener<ReqT>(delegate) {
            override fun onHalfClose() {
                try {
                    super.onHalfClose()
                } catch (e: Exception) {
                    throw RuntimeException(e)
                }

            }
        }
    }
}

[ejona86]

The new snippet looks fine.

It get's more complicated if you start looking at all the other places in the code, because we haven't verified that those code paths can definitely be reached, and how.

Virtually all of them can be reached. For each catch I would know very quickly if a user could trigger an exception within it. Most catches we have are to handle exceptions from application code. In our own code we do not typically throw exceptions.

but I would say the @Throws annotation is only helpful for those writing Kotlin shared libraries that they expect to be used in java projects.

I thought that was likely the case as well. In any case, unless an IDE or compiler helps you notice when the annotation is missing it is easy to forget. It sounds like the Kotlin compiler doesn't verify throws like the java compiler does (you can't override a method and add new checked exceptions).

[dapengzhang0]

As for the question of "do we want to support this", I think Kotlin developers should provide Java interoperability for their code if they use Java or want their code to be used by Java. And when using Kotlin APIs without Java interoperability explicitly announced, in general, users should add defensive boilerplate (as @tonicsoft did in the snippet) to prevent any potential risk of java interoperability issues.

cc @lowasser, this seems a problem in general for interoperation between Kotlin and Java.

[tonicsoft]

A completely different solution would be to add throws Exception to the signature of io.grpc.stub.ServerCalls.UnaryRequestMethod#invoke (ditto for streaming calls). Also up for discussion would be adding the same to the individual signatures of the generated XXXImplBase class.

There is a precedent for this approach: I'll use Netty as an example. ChannelInboundHandlerAdapter is the first class mentioned in the getting started guide, and it declares throws Exception on all of it's methods.

The benefit is you give control to the user whether they want to use checked exceptions or not.

For projects that consistently use checked exceptions to control the flow of the program and thus wouldn't want any to escape to GRPC, they can either omit the declaration in the implementation class (overridden function signatures can be tighter than the base class) or it could be a configurable feature of the protoc plugin.

For projects that consider checked exceptions an inconvenience (Guava propagate anyone?), having the throws Exception declaration allows them to be ignored.

[ejona86]

A completely different solution would be to add throws Exception to the signature of...

Nope. That is API-breaking. And we wouldn't want to introduce a new API just for this.

This sort of change would also need to happen to StreamObserver for the streaming use-case, and that forces all users to deal with the throws Exception even though gRPC itself won't throw an unchecked exception.

[ejona86]

it could be a configurable feature of the protoc plugin.

Also, we don't do any configuration like this in the protoc plugin. For multiple reasons you have to assume that all users are using one canonical version of the generated code. We could add an option to the .proto saying which behavior we wanted (so then all users see the same generated code), but that doesn't seem like it could be used very well in this case.

[tonicsoft]

That makes sense, thanks for the detailed reply once again.

[tonicsoft]

fwiw, here's an example of "sneaky throws" in the wild that I discovered today while doing something completely unrelated. I believe it's more evidence that GRPC should not be opinionated on this matter and should simply handle all Throwables, as the fact of the matter is that this is a feature of the JVM that people use, wisely or unwisely, Kotlin or no Kotlin:

https://github.com/testcontainers/testcontainers-java/blob/master/core/src/main/java/org/testcontainers/containers/PortForwardingContainer.java https://projectlombok.org/features/SneakyThrows

[jontro]

Thanks @tonicsoft ! Your latest solution solves my problem too. Using kotlin out of the box will make this problem appear.