Catch Exceptions thrown in LLRC callbacks to protect the client IO Reactor Thread

[jbaiera]

In the LLRC, we do not handle RuntimeExceptions thrown in the onFailure listener of a request callback.
So e.g. this code:

            restClient.performRequestAsync(request, new ResponseListener() {
                @Override
                public void onSuccess(Response response) {
                      // ...
                }

                @Override
                public void onFailure(Exception exception) {
                    throw new RuntimeException(exception);
                }
            });

will cause a broken client because the exception bubbles up in the IOReactor and it's being hard-shutdown as a result. This is something we should fix. It's always possible that due to some unforeseen state/bug/... an exception is thrown in the failure handler. While that shouldn't happen and callback code should be fixed, it's not right to simply silently shut down the io reactor here and fail subsequent requests (there isn't even logging in the example code above). Instead, we should probably catch all exceptions and worst case log a warning for the unhandled ones and not let them break the client.

Split off from #42133

[elasticmachine]

Pinging @elastic/es-core-features

[rsubramanyam2994]

We are observing a similar problem where the RestHighLevelClient is not catching IllegalStateException (See below for stack trace). Shouldn't RestHighLevelClient catch all exceptions (github link) and wrap them under an IOException or an ElasticsearchException?

j.l.IllegalStateException: Request cannot be executed; I/O reactor status: STOPPED
	at o.a.h.util.Asserts.check(Asserts.java:46)
	at o.a.h.i.n.c.CloseableHttpAsyncClientBase.ensureRunning(CloseableHttpAsyncClientBase.java:90)
	at o.a.h.i.n.c.InternalHttpAsyncClient.execute(InternalHttpAsyncClient.java:123)
	at o.e.c.RestClient.performRequestAsync(RestClient.java:531)
	at o.e.c.RestClient.performRequestAsyncNoCatch(RestClient.java:516)
	at o.e.c.RestClient.performRequest(RestClient.java:228)
	at o.e.c.RestHighLevelClient.internalPerformRequest(RestHighLevelClient.java:1593)
	at o.e.c.RestHighLevelClient.performRequest(RestHighLevelClient.java:1563)
	at o.e.c.RestHighLevelClient.performRequestAndParseEntity(RestHighLevelClient.java:1525)
	at o.e.c.IndicesClient.refresh(IndicesClient.java:527)

[jigarbjpatel]

Does anyone know what Version of ESClient is affected by this ? We are seeing this exception in 6.2.x but not in 6.4.0 version yet. Does that mean it is fixed in 6.4.0 version?

[jbaiera]

I have been spending some time in the HTTP Client code lately to try and understand what might cause the IOReactor to shutdown prematurely. I've tried to replicate throwing exceptions from the client in this manner but have not been successful in killing the IOReactor. I'm further convinced that this is not the cause because in other methods in the LLRC we may throw exceptions that are based on RuntimeException like deprecation warnings in Strict mode.

Instead, it seems that in the layers between the LLRC code and the IOReactor there are specific catch blocks that should protect the IOReactor from all runtime and checked Exceptions. This happens in the DefaultNHttpClientConnection.consumeInput method. It's more likely that the exceptions that are killing the IOReactor instances in the LLRC are due to NIO Failures of some sort. However, those failures are not clear. There is an open issue for trying to instrument the client to detect and log these IOReactor exceptions (#49124).

In the mean time, I'm going to close this out since I don't see much evidence that this may be the cause for failures.

[mwunderlich]

I have been struggling with this very issue for a while. It occurs when unit tests are being executed in our CI/CD system (TeamCity). Sometimes it also happens locally, but it is not reproducible in a deterministic manner in either environment (locally or on TC). I know the issue has been closed, but just to point out that this is still and issue (and hoping that it will get fixed as part of #49124).