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`).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
