DelayedStream.setStream() should cancel the provided stream if not using it

[ejona86]

If cancel() or setStream() was previously called then setStream() throws away the stream argument. It should cancel() the stream to make sure resources are freed, although I think only DelayedClientTransport benefits.

This should not be done until after #1536. I don't know if OkHttp suffers a similar problem. If so, we may want to revisit whether it makes sense to allow streams to be cancelled before start().

[ejona86]

This is more of a code health issue, since this shouldn't cause any trouble today. It's just a place where DelayedStream isn't doing the right thing in terms of the API. It is not believed to be an actual problem, because all the transports other than DelayedClientTransport only do an allocations in newStream; no registration. So there is no need to call cancel() with existing implementations if start() is never called.

But since that is implementation-specific, we'd like to clean it up. Moving to 1.2 though, since the change is a bit more invasive that I'd hope to do just before 1.1 release for something that shouldn't matter.

[zhangkun83]

#2618 didn't work out, because while it resolves the "real streams discarded" issue, it introduces a new issue that it may wastefully start only to cancel real streams immediately:

setStream() may be scheduled for an unbounded period of time into the future. For example, CallCredentials may take a long time to fetch the credentials before triggering setStream() (in MetadataApplierImpl), prior to which the RPC may be cancelled due to deadline-exceeded. In those cases, real stream are created before setStream() is called, only to be started (as required by cancel()) and then cancelled. start() typically involve I/O, e.g., sending headers, which are wasted. If the delay before setStream() is long enough, this will have a high chance to happen, unlike typical race conditions with low likelihood.

@ejona86 and I discussed three options to proceed with:

1. cancel() does not require start(). newStream() is allowed to take up memory or resources that would require cancel() to be released, which is what delayed transport is currently doing. Because of that, an unstarted stream must be cancelled before being discarded.
2. cancel() requires start(). newStream() is not allowed to take up memory or resources that would require cancel() to be released, which means an unstarted stream can be discarded without being cancelled. Netty and OkHttp transports are already doing so. Delayed transport is not, and needs to be changed, by moving the registration logic from newStream() to PendingStream.start(). We also need to check in-process transport.
3. cancel() requires start(), but we keep delayed transport as-is (as in the state of core: DelayedStream cancels provided stream if not using it. #2618), which means newStream() is allowed to take up memory or resources that would require cancel() to be released. To prevent the potential leak resulting from that, the callers of setStream() will not create the real stream if it finds out the delayed stream is already cancelled. There is still a race between checking the cancellation state of delayed stream, and the delayed stream being cancelled, but it has a much smaller window thus it won't cause many wasteful real stream start() calls.

We find Option 2 is the cleanest, and possibly the easiest. If that turns out to be not the case, we will try Option 3. This is not urgent. We will proceed once the LBv2 switch is done.

[carl-mastrangelo]

@zhangkun83 Is there anything more to do for this?

[carl-mastrangelo]

It looks like this didnt make it, re pushing.