Stop creating a CancellableContext for ClientCall

[ejona86]

As seen in #1878, it is pretty easy to accidentally create long chains of Contexts when we create a new Context for each ClientCall. Thinking about it more, it seems creating a Context was a misstep. The RPC doesn't really mean "a unit of work" in the same way as it does for server-side, and it is unlikely that any result processing should be killed when the RPC completes (successfully or unsuccessfully). Also, only async stub sees the context, which seems like further evidence that there is nothing inherent that should cause us to create the Context.

[johnbcoughlin]

We are using this Context to manage some shared state between interceptors on the same call. For example, one interceptor starts a stopwatch, and another interceptor uses ContextKey.STOPWATCH to log the elapsed time in the call so far. Certainly we can work around it if you do remove this context, just wanted to give one idea of how it might be used.

[ejona86]

@johnbcoughlin, your use case sounds fine. I wasn't very clear, but Context will still be supported, in that callbacks will still inherit from the Context as when the call started. What I'm suggesting removing is gRPC's creation of a new Context for each outgoing RPC.

Context interacts a bit strangely with the async stub when you do a new RPC in the callback of an RPC (like we were doing in some benchmarks and needed to avoid with 0021e06). Your case sounds like it will function fine, as long as the interceptors are consistent between calls and you don't have concerns about a long Context chain being formed (although we could optimize that case, as long as there aren't many CancellableContexts).

In the next release (0.15.0), you will also be able to use generic CallOption Options as a possible alternative to using Context for communicating with interceptors.

@johnbcoughlin, does that sound good? Right now we're thinking this change won't negatively impact anyone, so if you have a use-case for gRPC auto-creating a CancellableContext for outgoing RPCs I'd want to make sure it was addressed.

[johnbcoughlin]

@ejona86 it looks like I forgot how my own code works! We create the interceptionContext ourselves, so this change shouldn't affect us at all.

[buchgr]

@ejona86 do you suggest to also not create a new context when using a deadline?

[ejona86]

@buchgr, two options 1) handle the timeout manually (basically reverting back to earlier code using deadlineCancellationFuture) or 2) use a Context, but just hide it.

It looks like the code is actually broken right now because it doesn't cancel the Context when the RPC completes :-/. Note that even with option 1 we will still need to add a listener to the parentContext (or, rather, current context) in case it is cancelled.

[buchgr]

My concern was more with the deadline then not being propagated in nested RPC calls. I believe this was the original reason for using Context in ClientCall in the first place.

We could make it so that we only create a new Context if a deadline is specified or the parent context has a deadline.

It looks like the code is actually broken right now because it doesn't cancel the Context when the RPC completes :-/.

yes that's a bug! I ll take care of it.

[ejona86]

My concern was more with the deadline then not being propagated in nested RPC calls.

What caused us to change our mind is what does "nested RPC" even mean for a client? For servers it is pretty well defined. If you are using the Future interface or blocking interface, what is a "nested RPC"? If you are doing one RPC and then using the response to do another RPC, it seems that second RPC should have its own separate deadline.

If Context has a deadline, we will want to propagate it to the server (like we do now). But that doesn't mean we need to make a new CancellableContext.