[
https://issues.apache.org/jira/browse/TIKA-4886?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18113320#comment-18113320
]
ASF GitHub Bot commented on TIKA-4886:
--------------------------------------
Copilot commented on code in PR #3149:
URL: https://github.com/apache/tika/pull/3149#discussion_r3968654291
##########
tika-core/src/test/java/org/apache/tika/utils/ProcessUtilsTest.java:
##########
@@ -133,4 +134,21 @@ public void
testCheckCommandCustomTimeoutBoundsASlowCommand() {
assertTrue(elapsed < 4_000,
"checkCommandWithTimeout must honor its own timeout, not the
default; took " + elapsed + "ms");
}
+
+ @Test
+ public void testExecuteFailsFastIfTimeoutIsZero() throws Exception {
+ assumeFalse(SystemUtils.IS_OS_WINDOWS);
+
+ ProcessBuilder pb = new ProcessBuilder("sleep", "5");
+ ParseContext context = new ParseContext();
+ context.set(TimeoutLimits.class, new TimeoutLimits(0, 0));
+
+ long start = System.currentTimeMillis();
+ FileProcessResult result = ProcessUtils.execute(pb, context, 5_000L,
1000, 1000);
+ long elapsed = System.currentTimeMillis() - start;
+
+ assertTrue(result.isTimeout(), "a process with a 0 timeout should
timeout immediately without starting");
+ assertEquals(0, result.getGrantedTimeoutMillis(), "the process should
not have been granted any timeout; got " + result.getGrantedTimeoutMillis() +
"ms");
+ assertTrue(elapsed < 4_000, "fast path should return without spawning;
took " + elapsed + "ms");
Review Comment:
This test doesn’t actually verify that the subprocess was not started—on the
old behavior (start then immediately kill), `elapsed` could still be well under
4s, so the assertion may pass even if the process is spawned. To make this
regression-proof, use a command that would create an observable side effect if
it starts (e.g., `sh -c 'echo started > <tempfile>; sleep 5'`) and assert the
side effect did *not* occur when granted timeout is 0.
##########
tika-core/src/main/java/org/apache/tika/utils/ProcessUtils.java:
##########
@@ -442,4 +448,16 @@ public static boolean waitForWithHeartbeat(Process p,
ParseContext context, long
}
}
+ private static FileProcessResult failFastIfNoGrantedTimeout(long
requestedTimeoutMillis, long grantedTimeoutMillis) {
+ if (grantedTimeoutMillis <= 0) {
+ FileProcessResult result = new FileProcessResult();
+ result.isTimeout = true;
+ result.requestedTimeoutMillis = requestedTimeoutMillis;
+ result.grantedTimeoutMillis = grantedTimeoutMillis;
Review Comment:
The new behavior treats *any* `grantedTimeoutMillis <= 0` as an immediate
timeout. Please document (e.g., method-level comment) that negative values are
also considered 'no time budget' here, so future changes to timeout semantics
(if any) don’t accidentally reintroduce process spawning for `<= 0` grants.
##########
tika-core/src/main/java/org/apache/tika/utils/ProcessUtils.java:
##########
@@ -146,6 +146,12 @@ public static FileProcessResult execute(ProcessBuilder pb,
ParseContext context,
long grantedTimeoutMillis = context == null
? requestedTimeoutMillis
:
ParseTimeout.getOrCreate(context).budgetFor(requestedTimeoutMillis);
+
+ FileProcessResult result =
failFastIfNoGrantedTimeout(requestedTimeoutMillis, grantedTimeoutMillis);
+ if (result != null) {
+ return result;
+ }
Review Comment:
Returning `null` as a sentinel from `failFastIfNoGrantedTimeout` makes the
control flow easier to misuse later. Consider changing the helper to return a
boolean (and construct the `FileProcessResult` inline) or return an
`Optional<FileProcessResult>` to make the contract explicit.
##########
tika-core/src/main/java/org/apache/tika/utils/ProcessUtils.java:
##########
@@ -442,4 +448,16 @@ public static boolean waitForWithHeartbeat(Process p,
ParseContext context, long
}
}
+ private static FileProcessResult failFastIfNoGrantedTimeout(long
requestedTimeoutMillis, long grantedTimeoutMillis) {
+ if (grantedTimeoutMillis <= 0) {
+ FileProcessResult result = new FileProcessResult();
+ result.isTimeout = true;
+ result.requestedTimeoutMillis = requestedTimeoutMillis;
+ result.grantedTimeoutMillis = grantedTimeoutMillis;
+
+ return result;
+ }
+
+ return null;
Review Comment:
Returning `null` as a sentinel from `failFastIfNoGrantedTimeout` makes the
control flow easier to misuse later. Consider changing the helper to return a
boolean (and construct the `FileProcessResult` inline) or return an
`Optional<FileProcessResult>` to make the contract explicit.
##########
tika-core/src/test/java/org/apache/tika/utils/ProcessUtilsTest.java:
##########
@@ -133,4 +134,21 @@ public void
testCheckCommandCustomTimeoutBoundsASlowCommand() {
assertTrue(elapsed < 4_000,
"checkCommandWithTimeout must honor its own timeout, not the
default; took " + elapsed + "ms");
}
+
+ @Test
+ public void testExecuteFailsFastIfTimeoutIsZero() throws Exception {
+ assumeFalse(SystemUtils.IS_OS_WINDOWS);
+
+ ProcessBuilder pb = new ProcessBuilder("sleep", "5");
+ ParseContext context = new ParseContext();
+ context.set(TimeoutLimits.class, new TimeoutLimits(0, 0));
+
+ long start = System.currentTimeMillis();
+ FileProcessResult result = ProcessUtils.execute(pb, context, 5_000L,
1000, 1000);
+ long elapsed = System.currentTimeMillis() - start;
Review Comment:
Using `System.currentTimeMillis()` for elapsed-time assertions can be flaky
(wall-clock adjustments) and the 4s threshold is very loose for a 'fail fast'
path. Prefer `System.nanoTime()` for timing and consider a tighter bound (or
rely on an observable side effect as suggested) to reduce the chance of false
positives/negatives on CI.
##########
tika-core/src/test/java/org/apache/tika/utils/ProcessUtilsTest.java:
##########
@@ -133,4 +134,21 @@ public void
testCheckCommandCustomTimeoutBoundsASlowCommand() {
assertTrue(elapsed < 4_000,
"checkCommandWithTimeout must honor its own timeout, not the
default; took " + elapsed + "ms");
}
+
+ @Test
+ public void testExecuteFailsFastIfTimeoutIsZero() throws Exception {
+ assumeFalse(SystemUtils.IS_OS_WINDOWS);
+
+ ProcessBuilder pb = new ProcessBuilder("sleep", "5");
+ ParseContext context = new ParseContext();
+ context.set(TimeoutLimits.class, new TimeoutLimits(0, 0));
+
+ long start = System.currentTimeMillis();
+ FileProcessResult result = ProcessUtils.execute(pb, context, 5_000L,
1000, 1000);
+ long elapsed = System.currentTimeMillis() - start;
+
+ assertTrue(result.isTimeout(), "a process with a 0 timeout should
timeout immediately without starting");
+ assertEquals(0, result.getGrantedTimeoutMillis(), "the process should
not have been granted any timeout; got " + result.getGrantedTimeoutMillis() +
"ms");
+ assertTrue(elapsed < 4_000, "fast path should return without spawning;
took " + elapsed + "ms");
Review Comment:
Using `System.currentTimeMillis()` for elapsed-time assertions can be flaky
(wall-clock adjustments) and the 4s threshold is very loose for a 'fail fast'
path. Prefer `System.nanoTime()` for timing and consider a tighter bound (or
rely on an observable side effect as suggested) to reduce the chance of false
positives/negatives on CI.
> ProcessUtils spawns and immediately kills a subprocess when time budget is
> zero
> -------------------------------------------------------------------------------
>
> Key: TIKA-4886
> URL: https://issues.apache.org/jira/browse/TIKA-4886
> Project: Tika
> Issue Type: Bug
> Components: core
> Reporter: Tim Grein
> Priority: Minor
>
> Currently ProcessUtils.execute starts a subprocess, even if
> requestedTimeoutMillis is 0. TikaHttpClient on the other hand has a fail fast
> path, where a timeout exception is thrown, if the granted timeout is <= 0.
> ProcessUtils.execute should probably also have a fail fast path similar to
> the TikaHttpClient.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
