[
https://issues.apache.org/jira/browse/TIKA-4886?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18113258#comment-18113258
]
ASF GitHub Bot commented on TIKA-4886:
--------------------------------------
Copilot commented on code in PR #3149:
URL: https://github.com/apache/tika/pull/3149#discussion_r3967160808
##########
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 < 1000, "fast path should return without spawning;
took " + elapsed + "ms");
Review Comment:
The elapsed-time assertion is potentially flaky on slow/loaded CI and uses
`currentTimeMillis()` (wall clock) rather than a monotonic source. Consider
switching to `System.nanoTime()` for duration measurement and
relaxing/structuring the timing assertion (e.g., use a slightly higher
threshold or JUnit’s timeout utilities) to avoid intermittent failures
unrelated to behavior.
##########
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 checkIfExhausted(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:
The helper name `checkIfExhausted` is a bit ambiguous (it specifically
checks `grantedTimeoutMillis <= 0`, not general exhaustion conditions).
Consider renaming it to be explicit about the condition (e.g.,
`timeoutResultIfNoGrantedBudget` / `failFastIfNoGrantedTimeout`) so readers
immediately understand when/why this returns a non-null result.
##########
tika-core/src/test/java/org/apache/tika/utils/ProcessUtilsTest.java:
##########
@@ -18,6 +18,7 @@
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.junit.jupiter.api.Assertions.assertEquals;
Review Comment:
Static imports are typically kept in a consistent order (often alphabetical)
to reduce diff churn and improve scanability. Consider reordering these to
match the project’s import ordering conventions (e.g., `assertEquals`,
`assertFalse`, `assertTrue`).
> 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)
