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
Uncaught exceptions in server #6162
Comments
There's duplicate code in |
@dapengzhang0 replacing it with |
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 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. |
JVM cannot prevent a method with
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. |
We've not been able to use 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. |
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? |
Sorry, race condition. I see you already moved the discussion towards the Kotlin case 👍 |
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
The app is pure Kotlin (so my Kotlin implementation class is extending the generated |
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. |
https://kotlinlang.org/docs/reference/java-to-kotlin-interop.html#checked-exceptions |
@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 |
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
@ejona86 Thanks a lot for the tip. That's what happens when you copy code from SO... |
@ejona86 here's the new workaround, which has similar behaviour and is hopefully now safe:
|
The new snippet looks fine.
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.
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). |
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. |
A completely different solution would be to add There is a precedent for this approach: I'll use Netty as an example. 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 For projects that consider checked exceptions an inconvenience (Guava |
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 |
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. |
That makes sense, thanks for the detailed reply once again. |
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 https://github.com/testcontainers/testcontainers-java/blob/master/core/src/main/java/org/testcontainers/containers/PortForwardingContainer.java |
Thanks @tonicsoft ! Your latest solution solves my problem too. Using kotlin out of the box will make this problem appear. |
A cleanup regarding grpc#6162
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:
grpc-java/core/src/main/java/io/grpc/internal/ServerImpl.java
Line 558 in ba0fd84
You can see that
Exception
types that are not subclasses ofError
orRuntimeException
(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.
The text was updated successfully, but these errors were encountered: