Issue 355: ProcessHandler configurable executor timeout

[marklassau]

Allow an application developer to override the default Executor Timeout value

[kokorin]

1. Public static variables should be considered as bad practice.
2. A user should not know intrinsic details of ProcessHandler to change timeout

[marklassau]

Public static variables should be considered as bad practice.

The variable is private; it's the setter method that is public.
However, it seems you don't like the public static setter method either.
Clearly I don't agree that is necessary bad practice ... but it's your library so I am happy to make this work using whatever design principles you want to go with.

A user should not know intrinsic details of ProcessHandler to change timeout

The user has the intrinsic details of ProcessHandler thrust upon them the first time they see this in their logs:

WARN  [process.ProcessHandler] Executor hasn't stopped in 10000 millis, won't wait longer

It is only at this point that anyone will go looking for a way to change the timeout, so naturally they will look in ProcessHandler first - unless we change the log message or leave other clues in the code.

If you don't like the user working with ProcessHandler nor static methods then let me know if you have a preferred place that we could set a custom timeout.

Perhaps then somewhere within the fluent API when we start the ffmpeg process?

eg we could override the execute() method (and executeAsync())
eg:

FFmpeg.atPath()
        ...
        .executeWithTimeout(60_000);

Or we could create some sort of Config object if we want to leave room to add more configuration items in the future.

eg

ExecutorConfig executorConfig = new ExecutorConfig();
executorConfig.setTimeout(60_000);
FFmpeg.atPath()
        ...
        .execute(executorConfig);

Or if you were thinking something else let me know - I am happy to open a new PR that follows your preferred design approach.

[kokorin]

It's better to declare static final variable named DEFAULT_EXECUTOR_TIMEOUT_MILLIS and assign it to executorTimeoutMillis

[kokorin]

I prefer not to change execute and executeAsync methods. It's better to add another fluent method to set executor timeout.

[kokorin]

Please, rebase on latest in develop branch (as it contains MacOS test fixes)

[kokorin]

waitForExecutorToStop must be invoked, otherwise ProcessHandler exists immediately.
Check for zero timeout should be done in that method.

[marklassau]

EDIT: This reply was written when I had misunderstood your intention for executorTimeoutMillis == 0
I thought 0 would mean "do not wait at all", but now I think you intended 0 to mean "wait forever".
I still think that this comment is technically correct - there is a small (mostly harmless) edge-case bug.
But to understand the reasoning you will have to imagine that 0ms timeout means 0ms timeout ie "don't wait at all".

---

Let's discuss the implementation of waitForExecutorToStop().

First of all I think that this line

            if (System.currentTimeMillis() - waitStarted > timeoutMillis) {

has an edge-case bug.
It should actually use >=.

Think about the case where timeoutMillis == 0:

• System.currentTimeMillis() - waitStarted will almost certainly be 0
• The if clause will fail and you will end up invoking Thread.sleep(100)
• That would be unintended / unexpected

In a practical sense, this was not important when we had a constant EXECUTOR_TIMEOUT_MILLIS, and it is not important if we short-circuit for the timeoutMillis==0 case.
But given that we are discussing calling waitForExecutorToStop() for the zero case, I thought I should mention it.

[marklassau]

Second of all I wonder why you have a do ... while loop instead of a while loop.

The do ... while deals with the case where the executor has already stopped by the time we run waitForExecutorToStop().

In this case, why do we want to force a Thread.sleep(100)?
Why would we want to log "Executor hasn't yet stopped..." when that is not true?

The log is only at TRACE level, so the practical outcome is pretty harmless.
Further it is perhaps very unlikely to hit this case ... but it seems to me that a while loop would be the correct implementation - am I misunderstanding something?

[marklassau]

waitForExecutorToStop must be invoked, otherwise ProcessHandler exists immediately.

This was confusing me for a long time ... until I realised ... I think I misunderstood your previous request:

I think it makes sense to allow a user to disable timeout by setting 0 (zero) value.

What you meant was "a zero value means wait indefinitely".
I thought you meant "a zero value means don't wait at all".

As a general rule in configuration values, I prefer to let zero mean zero - and then use -1, or "any negative value" to mean a special case like "disable the timeout".
The bonus with "any negative value" is that you don't need to check for valid values - all values have some meaning.

However, in this particular case, would an actual 0ms timeout ever be a useful setting?
Perhaps here the "0" as a special value makes sense ...

[kokorin]

However, in this particular case, would an actual 0ms timeout ever be a useful setting? Perhaps here the "0" as a special value makes sense ...

Even ffmpeg itself uses -threads 0 as an option to use all threads available.

Second of all I wonder why you have a do ... while loop instead of a while loop.
The do ... while deals with the case where the executor has already stopped by the time we run waitForExecutorToStop().

To be honest, I don't care. It's highly unlikely that ffmpeg will complete in 100ms or less.

if (System.currentTimeMillis() - waitStarted > timeoutMillis) {

I think it can be like if (timeoutMillis> 0 && System.currentTimeMillis() - waitStarted > timeoutMillis) {

[kokorin]

Thank you