Concurrency Bug between isAlive() and exitValue() in ManagedProcess and/or in DefaultExecuteResultHandler

[vorburger]

enola-dev/enola#163 may be due to a Concurrency Bug between isAlive() and exitValue() ?

In ManagedProcess#waitForExitMaxMsWithoutLog() there is currently this:

if (maxWaitUntilReturning != -1) {
    resultHandler.waitFor(maxWaitUntilReturning);
    checkResult();
    if (!isAlive()) {
        return exitValue();
    }
    return INVALID_EXITVALUE;
}

My original thinking was probably that because checkResult() does resultHandler.hasResult() this should be safe? But on 2nd thought, 10 years 😄 later, I wonder:

• Firstly, why not just check for hasResult() (or, better, add a new hasExitValue()) instead of !isAlive()
• secondly, if there could be a concurrency issue wherein the DefaultExecuteResultHandler x3 volatile fields hasResult and exitValue and exception are not set "atomically"... that could be written better.

Also, it's probably better to introduce separate new special values for INVALID_EXITVALUE instead of using that for several things would also make debug clearer. Or, probably better, just throw a new exception, in case of such clear inconsistent states - that INVALID_EXITVALUE is more confusing that really providing any useful feedback to a caller.

I'll raise a PR with some work around this - and see if it helps me to stop runing into enola-dev/enola#163 while using https://docs.enola.dev/use/execmd.

@mosesn (from #103) and @cardamon (from #96) and @duttonw FYI.

[vorburger]

Oh wait I forgot DefaultExecuteResultHandler is actually from package org.apache.commons.exec and not mine...

... I will just replace it with a new AtomicExecuteResultHandler of our own in an upcoming PR...

[vorburger]

Closing this issue, hoping this will work better (or at least lead to clearer errors?) with #110 + #112.

I'll release this as 3.1.5 (maybe together with #45)