Copilot commented on code in PR #3252:
URL: https://github.com/apache/tika/pull/3252#discussion_r4203081736
##########
tika-core/src/test/java/org/apache/tika/parser/mock/MockParser.java:
##########
@@ -370,11 +370,11 @@ private void hang(Node action) {
throw new RuntimeException(
"Must specify attribute \"pulse_millis\" if the hang
is \"heavy\"");
}
- String pulseMillisString = mNode.getNodeValue();
+ String pulseMillisString = pNode.getNodeValue();
try {
pulseMillis = Long.parseLong(pulseMillisString);
} catch (NumberFormatException e) {
- throw new RuntimeException("Value for \"millis\" attribute
must be a long.");
+ throw new RuntimeException("Value for \"pulse_millis\"
attribute must be a long.");
Review Comment:
The added `MockParserTest` cases exercise only non-heavy hangs, so they
still pass if this line regresses to reading `millis` again. Add a heavy,
interruptible hang with a long `millis` and short `pulse_millis`, then
interrupt it and assert prompt termination so the corrected attribute lookup is
actually pinned.
##########
tika-eval/tika-eval-app/src/test/java/org/apache/tika/eval/app/TikaEvalCLITest.java:
##########
@@ -64,102 +61,75 @@ public static void setUp() throws Exception {
reportProfile();
}
- private static void compare() throws IOException {
+ private static void compare() throws Exception {
List<String> args = new ArrayList<>();
args.add("Compare");
args.add("-a");
- args.add(ProcessUtils.escapeCommandLine(extractsDir
+ args.add(extractsDir
.resolve("extractsA")
.toAbsolutePath()
- .toString()));
+ .toString());
args.add("-b");
- args.add(ProcessUtils.escapeCommandLine(extractsDir
+ args.add(extractsDir
.resolve("extractsB")
.toAbsolutePath()
- .toString()));
+ .toString());
args.add("-d");
- args.add(ProcessUtils.escapeCommandLine(compareDBDir
+ args.add(compareDBDir
.toAbsolutePath()
- .toString() + "/" + dbName));
+ .toString() + "/" + dbName);
- execute(args, 60000);
+ execute(args);
}
- private static void profile() throws IOException {
+ private static void profile() throws Exception {
List<String> args = new ArrayList<>();
args.add("Profile");
args.add("-e");
- args.add(ProcessUtils.escapeCommandLine(extractsDir
+ args.add(extractsDir
.resolve("extractsA")
.toAbsolutePath()
- .toString()));
+ .toString());
args.add("-d");
- args.add(ProcessUtils.escapeCommandLine(profileDBDir
+ args.add(profileDBDir
.toAbsolutePath()
- .toString() + "/" + dbName));
- execute(args, 60000);
+ .toString() + "/" + dbName);
+ execute(args);
}
- private static void reportProfile() throws IOException {
+ private static void reportProfile() throws Exception {
List<String> args = new ArrayList<>();
args.add("Report");
args.add("-db");
- args.add(ProcessUtils.escapeCommandLine(profileDBDir
+ args.add(profileDBDir
.toAbsolutePath()
- .toString() + "/" + dbName));
+ .toString() + "/" + dbName);
args.add("-rd");
- args.add(ProcessUtils.escapeCommandLine(profileReportsDir
+ args.add(profileReportsDir
.toAbsolutePath()
- .toString()));
- execute(args, 60000);
+ .toString());
+ execute(args);
}
- private static void reportCompare() throws IOException {
+ private static void reportCompare() throws Exception {
List<String> args = new ArrayList<>();
args.add("Report");
args.add("-db");
- args.add(ProcessUtils.escapeCommandLine(compareDBDir
+ args.add(compareDBDir
.toAbsolutePath()
- .toString() + "/" + dbName));
+ .toString() + "/" + dbName);
args.add("-rd");
- args.add(ProcessUtils.escapeCommandLine(compareReportsDir
+ args.add(compareReportsDir
.toAbsolutePath()
- .toString()));
- execute(args, 60000);
+ .toString());
+ execute(args);
}
- private static void execute(List<String> incomingArgs, long maxMillis)
throws IOException {
- List<String> args = new ArrayList<>();
- String cp = System.getProperty("java.class.path");
- args.add("java");
- args.add("-Djava.awt.headless=true");
- args.add("-cp");
- args.add(cp);
- args.add("org.apache.tika.eval.app.TikaEvalCLI");
- args.addAll(incomingArgs);
-
- ProcessBuilder pb = new ProcessBuilder(args);
- pb.redirectOutput(ProcessBuilder.Redirect.INHERIT);
- pb.redirectError(ProcessBuilder.Redirect.INHERIT);
- Process process = pb.start();
- long started = System.currentTimeMillis();
- long elapsed = System.currentTimeMillis() - started;
- int exitValue = Integer.MIN_VALUE;
- while (elapsed < maxMillis && exitValue == Integer.MIN_VALUE) {
- try {
- exitValue = process.exitValue();
- } catch (IllegalThreadStateException e) {
- //swallow
- }
- elapsed = System.currentTimeMillis() - started;
- }
- if (exitValue == Integer.MIN_VALUE) {
- process.destroy();
- throw new RuntimeException("Process never exited within the
allowed amount of time.\n" + "I needed to destroy it");
- }
+ private static void execute(List<String> args) throws Exception {
+ TikaEvalCLI.main(args.toArray(new String[0]));
Review Comment:
This in-process call removes the previous 60-second hard limit. If Compare,
Profile, or Report deadlocks, `@BeforeAll` can now block the entire test JVM
indefinitely; keep a bounded execution mechanism (or a JUnit timeout with
reliable cleanup) while avoiding the per-command fork.
##########
tika-pipes/tika-pipes-integration-tests/src/test/java/org/apache/tika/pipes/core/SharedServerModeTest.java:
##########
@@ -512,120 +297,29 @@ public void testMultipleOomsWithRecovery(@TempDir Path
tmp) throws Exception {
PipesConfig pipesConfig = PipesConfig.load(tikaJsonConfig);
try (PipesParser pipesParser = PipesParser.load(tikaJsonConfig,
pipesConfig)) {
- for (int i = 0; i < 3; i++) {
- // Trigger OOM
- PipesResult oomResult = pipesParser.parse(new FetchEmitTuple(
- "oom" + i + ".xml",
- new FetchKey(FETCHER_NAME, "oom" + i + ".xml"),
- new EmitKey(EMITTER_NAME, ""),
- new Metadata(),
- new ParseContext(),
- FetchEmitTuple.ON_PARSE_EXCEPTION.SKIP));
- assertEquals(PipesResult.RESULT_STATUS.OOM, oomResult.status(),
- "OOM " + i + " should return OOM status");
+ assertTrue(parse(pipesParser, "warmup.xml").isSuccess(), "warmup
should succeed");
+ int port = pipesParser.getCurrentServerPort();
+ assertTrue(port > 0, "should have a port after warmup");
- // Verify recovery
- PipesResult okResult = pipesParser.parse(new FetchEmitTuple(
- "ok" + i + ".xml",
- new FetchKey(FETCHER_NAME, "ok" + i + ".xml"),
- new EmitKey(EMITTER_NAME, ""),
- new Metadata(),
- new ParseContext(),
- FetchEmitTuple.ON_PARSE_EXCEPTION.SKIP));
- assertTrue(okResult.isSuccess(),
- "After OOM " + i + ", recovery should succeed. Got: "
+ okResult.status());
+ for (int i = 0; i < 3; i++) {
+ assertEquals(PipesResult.RESULT_STATUS.OOM, parse(pipesParser,
"oom" + i + ".xml").status(),
+ "OOM " + i);
+ PipesResult okResult = parse(pipesParser, "ok" + i + ".xml");
+ assertTrue(okResult.isSuccess(), "after OOM " + i + ": " +
okResult.status());
+ assertEquals("Test Author",
okResult.emitData().getMetadataList().get(0).get("dc:creator"));
+
+ int newPort = pipesParser.getCurrentServerPort();
+ assertTrue(newPort > 0 && newPort != port,
+ "server must restart on a new port after OOM " + i +
": " + port + " -> " + newPort);
Review Comment:
Binding a replacement server to port 0 does not guarantee a different port
from the closed listener (and the server enables `SO_REUSEADDR`), so this can
fail even when every OOM correctly starts a new JVM. Assert the parser
generation/start count increases once per OOM instead; that directly proves
replacement without depending on OS ephemeral-port selection.
--
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]
