blocking v2 client stub inconsistent about StatusRuntimeException verus StatusException

[benjaminp]

io.grpc.stub.BlockingClientCall uses the checked StatusException, so server-streaming and client-streaming v2 blocking RPCs have to deal with StatusException. However, the unary blocking v2 RPCs stubs have the implementation and thus behavior as v1; namely, all status exceptions are StatusRuntimeException. Should this be made consistent?

[kannanjgithub]

The reason that we started using the checked exception is that we want to start moving in general to StatusException rather than StatusRuntimeException and the read/write methods in the BlockingClientCall already had other checked exceptions so it wasn't making things easier on the user if the status was unchecked.
ClientCalls and the old blocking stub are stable so we can't change from StatusRuntimeException to a checked exception even if we wanted to since we can't break existing code. The only way we could have the checked StatusException be thrown by unary blocking RPCs to to introduce new wrapper methods in BlockingClientCall that would catch and convert StatusRuntimeException to StatusException, however it doesn't appear like this is necessarily what you want.

[benjaminp]

I'm just curious why this "blocking v2" interface is inconsistent. If the goal is to move to StatusException in general, why not make all blocking methods on the new interface throw the checked StatusException? Whether that requires adding new wrappers or not seems like an implementation detail.

[kannanjgithub]

The commit that introduced BlockingClientCall was for creating blocking methods used by ClientCalls that is in turn used by the generated stub code (example).

Can you clarify what are the other blocking methods in the v2 interface you are referring to that don't throw StatusException?

[benjaminp]

Can you clarify what are the other blocking methods in the v2 interface you are referring to that don't throw StatusException?

As the OP says, unary methods on the v2 interface use the same implementation as the v1 interface and therefore report errors with StatusRuntimeException.

[kannanjgithub]

Figured you are asking not to make the already stable ClientCalls.blockingUnaryCall throw StatusException but why not make the new wrapper stub (this is what is v2)'s unaryRpc method catch and convert StatusRuntimeException to StatusException.

public static final class SimpleServiceBlockingV2Stub
      extends io.grpc.stub.AbstractBlockingStub<SimpleServiceBlockingV2Stub> {
  public io.grpc.testing.protobuf.SimpleResponse unaryRpc(io.grpc.testing.protobuf.SimpleRequest   request) {
      return io.grpc.stub.ClientCalls.blockingUnaryCall(
          getChannel(), getUnaryRpcMethod(), getCallOptions(), request);
  }
}

[kannanjgithub]

We could make a new ClientCalls.blockingV2UnaryCall() that throws the checked StatusException.

[benjaminp]

We could make a new ClientCalls.blockingV2UnaryCall() that throws the checked StatusException.

Makes sense to me.

[ejona86]

API review meeting:

• We do want to swap to a checked exception for unary in blocking stub v2
• That will be a breaking change to new users of the v2 stub, but they can use the older codegen with the newer runtime as they work to upgrade to the new signature. Users can also use the v1 stub until they get past the breakage
• In the short-term, to let this get done quickly and to avoid further increasing the number of users impacted, we'll probably put a method in ClientCalls that just catches StatusRuntimeException and converts it to StatusException. But medium term we should avoid creating two exceptions by making something a bit more native.