stub,core: avoid calling onready if the call is UNARY or SERVER_STREAMING

[carl-mastrangelo]

No description provided.

[carl-mastrangelo]

benchmarks to come

[ejona86]

Won't this class generally extend ForwardingClientCallListener (because there's at least one interceptor) and so always return true?

[carl-mastrangelo]

We extend in in ManagedChannelImpl, but that can be undone (ServiceConfigInterceptor does, but that can be removed).

I'm okay with leaving interceptors a little slower for now. To avoid too much invasion, we need to have the interceptor return a mask of what callbacks it wants. That would be a slightly bigger change. ForwardingClientCallListener and SimpleForwardingClientCallListener could work with the mask, if delegate() is not overridden (and thus cannot return different instances).

[ejona86]

A change that doesn't work with interceptors is useless to me. There's no point in bothering, as almost no users would benefit.

If this is a POC to say "hey, look at how much faster this is, we should turn this into something real," then that's fine. I do believe avoiding the onReady callback could provide noticeable gains. But extending a solution to work with interceptors seems non-trivial.

Now maybe we could side-step this problem for unary RPCs. With our optimizations in place, there's really no point in onReady for them, as the call will already be halfClosed by that point.

[carl-mastrangelo]

I thought about this over the weekend. Three options came to mind:

1. Add a bitmask to ClientCall, which allows it to express what callbacks it wants. The default can be all events, which we can narrow in our implementations. I was thinking of exposing it as an int callbackMask() { return ~1; } on ClientCall.
2. Make a CallOption like NO_ONREADY and pass it in at call creation.
3. Declare that UNARY and CLIENT_STREAMING never will get an onReady (or, get it unconditionally, possibly after headers). I think this is my favorite, but slightly risky.

Do you have preference, or alternatives?

[ejona86]

(1) doesn't seem like it'd work, because we don't have access to the ClientCall. It'd need to be on the ClientCall.Listener. We could maybe call the method immediately within start().

I'm not wild about (2).

(3) is basically what I was suggesting, although it would be SERVER_STREAMING, not CLIENT_STREAMING. Our documentation already says it may not happen on unary-request RPCs. https://grpc.github.io/grpc-java/javadoc/io/grpc/ClientCall.Listener.html#onReady--

[carl-mastrangelo]

So, two options for number 3: Don't fire an onReady() event, or unconditionally fire it along with another event (such as headersRead).

The first is much simpler, but may break existing user than may have depended on this. The second is less correct, but may avoid hangs for clients who (wrongly) expect it.

Thoughts?

[carl-mastrangelo]

I have updated the code, and actually id have to change some tests that expected this.

[ejona86]

Don't forget to reword the commit title.