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