Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue 355: ProcessHandler configurable executor timeout #356

Merged
merged 2 commits into from
Sep 10, 2023
Merged

Issue 355: ProcessHandler configurable executor timeout #356

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0d4817e
Issue 355: Allow an application developer to override the default Exe…
marklassau Sep 5, 2023
ba0d06b
Issue 355: PR feedback: use int param instead of allowing nulls + 0 t…
marklassau Sep 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 20 additions & 1 deletion src/main/java/com/github/kokorin/jaffree/ffmpeg/FFmpeg.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ public class FFmpeg {

private LogLevel logLevel = LogLevel.INFO;
private String contextName = null;
private Integer executorTimeoutMillis = null;

private final Path executable;

Expand Down Expand Up @@ -387,6 +388,23 @@ public FFmpeg setContextName(final String contextName) {
return this;
}

/**
* Overrides the default {@link com.github.kokorin.jaffree.process.Executor} timeout.
* <p>
* Most normal use cases will easily complete within the default timeout. It is not recommended
* to set an explicit timeout value unless you have actually experienced unwanted timeouts.
*
* @param executorTimeoutMillis the custom executor timeout in milliseconds
* @return this
*/
public FFmpeg setExecutorTimeoutMillis(final int executorTimeoutMillis) {
if (executorTimeoutMillis < 0) {
throw new IllegalArgumentException("Executor timeout cannot be negative");
}
this.executorTimeoutMillis = executorTimeoutMillis;
return this;
}

/**
* Starts synchronous ffmpeg execution.
* <p>
Expand Down Expand Up @@ -484,7 +502,8 @@ protected ProcessHandler<FFmpegResult> createProcessHandler() {
.setStdErrReader(createStdErrReader(outputListener))
.setStdOutReader(createStdOutReader())
.setHelpers(helpers)
.setArguments(buildArguments());
.setArguments(buildArguments())
.setExecutorTimeoutMillis(executorTimeoutMillis);
}

/**
Expand Down
25 changes: 23 additions & 2 deletions src/main/java/com/github/kokorin/jaffree/process/ProcessHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,8 +49,9 @@ public class ProcessHandler<T> {
private List<ProcessHelper> helpers = null;
private Stopper stopper = null;
private List<String> arguments = Collections.emptyList();
private int executorTimeoutMillis = DEFAULT_EXECUTOR_TIMEOUT_MILLIS;

private static final int EXECUTOR_TIMEOUT_MILLIS = 10_000;
private static final int DEFAULT_EXECUTOR_TIMEOUT_MILLIS = 10_000;
private static final Logger LOGGER = LoggerFactory.getLogger(ProcessHandler.class);

/**
Expand Down Expand Up @@ -120,6 +121,24 @@ public synchronized ProcessHandler<T> setArguments(final List<String> arguments)
return this;
}

/**
* Overrides the default Executor timeout.
* <p>
* A null value is interpreted as "use default timeout".
*
* @param executorTimeoutMillis the new Executor timeout in milliseconds
* @return this
*/
public ProcessHandler<T> setExecutorTimeoutMillis(final Integer executorTimeoutMillis) {
kokorin marked this conversation as resolved.
Show resolved Hide resolved
if (executorTimeoutMillis != null) {
if (executorTimeoutMillis < 0) {
throw new IllegalArgumentException("Executor timeout cannot be negative");
}
this.executorTimeoutMillis = executorTimeoutMillis;
}
return this;
}

/**
* Executes a program.
* <p>
Expand Down Expand Up @@ -180,7 +199,9 @@ protected T interactWithProcess(final Process process) {
status = process.waitFor();
LOGGER.info("Process has finished with status: {}", status);

waitForExecutorToStop(executor, EXECUTOR_TIMEOUT_MILLIS);
if (executorTimeoutMillis > 0) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@marklassau marklassau Sep 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@marklassau marklassau Sep 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ...

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) {

waitForExecutorToStop(executor, executorTimeoutMillis);
}
} catch (InterruptedException e) {
LOGGER.warn("Process has been interrupted");
if (stopper != null) {
Expand Down