[ML] Avoid 5s wait in AbstractNativeProcessTests

[davidkyle]

Running the unit tests I've noticed AbstractNativeProcessTests often takes a long time to complete, individual methods either complete very quickly or take a little over 5 seconds suggesting something is timing out.

Indeed the 5 second wait is in the close() method

elasticsearch/x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/process/AbstractNativeProcess.java

Line 195 in 68817d7

logTailFuture.get(5, TimeUnit.SECONDS);

When used in a try-with-resource block close() is called before mockNativeProcessLoggingStreamEnds.countDown() so the call to wait for the log tail future to finish always times out as it is waiting on the mockNativeProcessLoggingStreamEnds latch to countdown.

Closes #37339

[elasticmachine]

Pinging @elastic/ml-core (Team:ML)

[przemekwitek]

LGTM

[droberts195]

I think this could be made even better by nesting the finally so that we count down before close whether the test passes or fails.

        try (AbstractNativeProcess process = new TestNativeProcess()) {
            try {
                process.start(executorService);
            } finally {
                mockNativeProcessLoggingStreamEnds.countDown();
                // Not detecting a crash is confirmed in terminateExecutorService()
            }
        }

(And obviously the same pattern for all the other places too.)

Or to avoid nested try blocks it could be done like we're already doing it in testStart_DoNotDetectCrashWhenProcessIsBeingKilled:

        AbstractNativeProcess process = new TestNativeProcess()) {
        try {
            process.start(executorService);
        } finally {
            mockNativeProcessLoggingStreamEnds.countDown();
            // Not detecting a crash is confirmed in terminateExecutorService()
            process.close();
        }

[davidkyle]

Yes if the test fails or throws before mockNativeProcessLoggingStreamEnds.countDown(); the 5s wait will be hit again. I've optimised for the common case and the fix was pleasingly simple so I didn't go any further.

[droberts195]

If you can't be bothered then let me push a change while the PR is open, as at least that will save the effort of two PRs and two backports.

[davidkyle]

I pushed the nested try

[droberts195]

Thanks ❤️

[droberts195]

LGTM

[elasticsearchmachine]

💚 Backport successful

Status	Branch	Result
✅	7.x