Executor usage in ClientCallImpl races with channel shutdown and termination.

[zhangkun83]

ManagedChannelImpl clear scheduledExecutor in shutdown(), and releases (which potentially closes) executor in maybeTerminateChannel().

Neither newCall() nor ClientCallImpl checks the shutdown state of the channel. ClientCallImpl relies on FailingClientTransport for the expected behavior. However, ClientCallImpl uses the passed in executors anyway, for scheduling the deadline timer and invoking the call listener.

If ClientCallImpl tries to schedule a deadline timer after the channel is shut down, it will get a NPE. If it runs the call listener after the shared executor has been closed, which is 1 second (SharedResourceHolder.DESTROY_DELAY_SECONDS) after all references are gone, e.g., the application calls Call.start() that late, it will get a RejectedExecutionException. Our current tests are not testing for the two cases.

This doesn't seem to be a serious issue. It only affect people who try to use Calls after the channel has been shutdown. I am yet to figure out a solution.

Anyway, it seems executor should be cleared after being returned to the shared pool, like scheduledExecutor.

[zhangkun83]

We should not miss out the case where a new call is created, the channel is terminated, then the call is started. Simply resetting the executors in channel upon termination doesn't help with this case, because the call holds on to the executors.

[zhangkun83]

OobChannel has the same issue with ManagedChannelImpl.

[zhangkun83]

@ejona86 @dapengzhang0, I wonder if the fix should be:

1. Only release the executors when channel is terminated
2. newCall() and call.start() throw if channel is shutdown. Calling them after shutdown() is application's fault. They should and can consciously avoid it. This might be considered an API-breaking change though.

[ejona86]

@zhangkun83, I'm not sold on (2), but even if I was favorable toward it, it does seem like an API breakage.