Catch Throwable instead Exception or RuntimeException&Error

[jonnyzzz]

While debugging my code problems I spot several places in the library code that might lead to uncatched/unreported exceptions being thrown through catch blocks.

In SerializingExcutor.TaskRunner#run there is try { .. } catch (RuntimeException e) { .. } block. It could easily happen any other Exception or Error can be thrown. (since checked exceptions are not necessarily on bytecode level, an Exception types are also possible, no matter they are not declared in Java code).

In the ServerImpl I see slightly incorrect check, that catches RuntimeException and Error only. I suppose a bit correct is to catch Throwable there too. In messageRead, streamCreated, halfClosed methods.

There are a few more places I found, e.g. in RouteGuideActivity, RouteGuideClient, AsyncFrameWriter.WriteRunnable#run, GRPCUtil#TIMER_SERVICE, MessageFramer,
ManagedChannelProvider#isAndroid, ClientInterceptors.CheckedForwardingClientCall#start, NameResolverProvider#isAndroid,

In internal/Util it is not clear if Throwable should be checked

[ejona86]

Just to make sure I understand, this issue is more "theoretical", right? You didn't encounter a thrown Error/Throwable that broke things but should have been recoverable?

It could easily happen any other Error can be thrown.

Any time I've ever seen an unhandled Error, the system has been in an unrecoverable state. (Some things like JVM containers can do a reasonable job of cleaning up after these cases, but those are in a very different position than gRPC.) There is a ton of code that doesn't "handle" OutOfMemoryError properly, and plenty that can't handle it properly. The definition of an error is "An Error is a subclass of Throwable that indicates serious problems that a reasonable application should not try to catch."

All that is to say, that handling Error, in general, feels like a fool's errand. There are some specific cases where that isn't true, like when dealing with reflection/class loading/native code. But we do go through some effort to handle it in high likelihood, centralized places.

In the SerializingExecutor case it will be "handled" by the Executor, which honestly makes me feel much better than just logging it, since it can call the UncaughtExceptionHandler and System.exit(). Although we are catching Throwable in the direct case. But that's a bit of a different case.

Maybe I should investigate more my theory that it is probably mostly safe to call UncaughtExceptionHandler even in places that don't adhere to the "thread terminates" part of the API, like in these executors.

(since checked exceptions are not necessarily on bytecode level, an Exception types are also possible, no matter they are not declared in Java code).

I don't have much sympathy for this. To rethrow such an Exception would require the same sort of hack that caused it in the first place (with the exception of a few reflect-related methods which can cause it "naturally").

In the same way I probably wouldn't have much sympathy for someone throwing/extending Throwable directly.

In the ServerImpl I see slightly incorrect check, that catches RuntimeException and Error only.

That, like many other places you're probably noticing this, is because it re-throws. We used to use propagateIfPossible/throwIfPossible from Guava (it went through some tweaks/renames) in more places, but due to Guava changes some of them needed to be removed. But catching Throwable directly also disables checked Exception handling, which is much more worrisome in many parts of code than the risk of someone throw new Throwable().

[jonnyzzz]

Thank you. I agree with you on the point that most of such exceptions are hard to be handled in the grpc code. Still, I expect grpc-java to work correctly, no matter what disaster happens in the callback.

Under work correctly, I mean it will not leak stale GRPC connection, for example, or it will not leak a Netty-related connection (HTTP stream / TCP connect / etc). It's definitely expected to crash anything and fail (not to kill the process, btw, since not everyone is able to handle that correctly, sadly). Also, it's nice to make it fail on the other end too.

I came across this potential, or "theoretical" problem while debugging my code that was dealing with grpc library both for the client and for the server.

Does it look to you that the code fails correctly after such theoretical and unhandled exceptions?

[keshy]

this is actually still happening causing a crash. I have a server project that am using google cloud logging sdk that pulls in dependencies in an awkward manner causing it to fail.

            dependency group: 'com.google.cloud', name: 'google-cloud', version: '0.8.0'
            dependency group: 'io.grpc', name: 'grpc-okhttp', version: '1.10.1'
            dependency group: 'com.google.cloud', name: 'google-cloud-logging', version: '1.35.0'

This causes the code to load as

@Internal
public final class OkHttpChannelProvider extends ManagedChannelProvider {

  @Override
  public boolean isAvailable() {
    return true;
  }

  @Override
  public int priority() {
    return (GrpcUtil.IS_RESTRICTED_APPENGINE || isAndroid()) ? 8 : 3;
  }

  @Override
  public OkHttpChannelBuilder builderForAddress(String name, int port) {
    return OkHttpChannelBuilder.forAddress(name, port);
  }

Can you suggest a work around if any?

What I get is this:

Caused by: java.lang.NoSuchMethodError: io.grpc.okhttp.OkHttpChannelProvider.isAndroid()Z
	at io.grpc.okhttp.OkHttpChannelProvider.priority(OkHttpChannelProvider.java:36)
	at io.grpc.ManagedChannelProvider$1.getPriority(ManagedChannelProvider.java:49)

[ejona86]

@keshy, I don't think your issue is related to this issue. NoSuchMethodError of that type can completely break any program, independent of what it does to handle the exception. I suggest looking at your dependencies to make sure the dependencies on grpc match. When using gradle, you can use gradle dependencies to investigate the versions used. Please open a separate issue if you have further comments.

[dapengzhang0]

Closing. Feel free to reopen if you have more input or the issue need be further addressed.

[m7onov]

One of the reason to make ServerImpl catch Throwable is to support Kotlin lang. In Kotlin there are no compile time checks for checked exceptions. So imagine the case: you implement grpc service in kotlin, call some java code in it that throws checked exception. Compiles perfectly. But in runtime ServerImpl will get Exception which is not descendant of RuntimeException or Error. Then we get channel leak. And this is not theoretical case

[ejona86]

@m7onov thanks for the information about Kotlin behavior. Opened #4864 for the Kotlin issue.