Go to "permanent error mode" if ChannelExecutor throws exception

[dapengzhang0]

As mentioned in discussion in #3288, it's questionable only catching instead of throwing exceptions in ChannelExecutor. Should we throw regardless or should we only log in production and throw in test evn?

[dapengzhang0]

@zhangkun83 , @ejona86

[ejona86]

I do think it tends to make sense to catch and log the exception, since propagating the exception will probably only cause additional failures and we know the current stack does not depend on the result of the call (since the execution is not guaranteed to be complete when drain returns.)

I don't think we should throw in a test environment; that's likely to produce confusion. Instead, you can inject a "exception handler" that is passed the exception. Normally, it would simply log. In tests it could save the exception and we can add an @After to assert that no exception was thrown. None of the tests should trigger the exception code path (except maybe a test that is testing the exception code path :) ).

[zhangkun83]

Instead, you can inject a "exception handler" that is passed the exception

Not sure if you meant injecting the exception handler to ChannelExecutor, but I am just thinking about that. ManagedChannelImpl, when capturing a throwable, may switch to an "permanent error mode" which would fail subsequent RPCs, which propagate the error very loudly.

[carl-mastrangelo]

FYI, I have been looking at consolidateing our executors (we have many) including the channel exec. If you make a change before I send out mine, please cc me.

[ejona86]

I'm fine with going into a "permanent error mode" if we get an exception with ChannelExecutor. Although the implementation will need to be very simple.

[ejona86]

@zhangkun83, assigning this to you since it seemed you were really interested in getting this done; feel free to unassign/reassign.

[ejona86]

#4152 disabled the added panic mode. It doesn't delete it, but leaves it non-functional.

We discovered pre-existing tests that had failures cause by exitIdleMode scheduling a timer, which can trigger Thread creation. The timer is to schedule IDLE_TIMEOUT, which is enabled by default to 30 minutes. In certain test environments, Thread creation is black-listed and throws an exception. Previously the exception was caught and logged, but otherwise things worked fine for the test; the test would not last long enough such that the broken IDLE_TIMEOUT would matter.

I thus disabled panic mode so that we can fix up those tests before re-enabling. These tests would all be using InProcessTrasport, and since the recent #4139 it is now possible to pass your own ScheduledExecutorService. These tests could be fixed by passing a fake service of some sort, like TestingExecutors.noOpScheduledExecutor() in Guava's testlib.