[
https://issues.apache.org/jira/browse/TIKA-4929?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18124257#comment-18124257
]
ASF GitHub Bot commented on TIKA-4929:
--------------------------------------
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.
> Stream line pipes/server tests in 4.2.x
> ---------------------------------------
>
> Key: TIKA-4929
> URL: https://issues.apache.org/jira/browse/TIKA-4929
> Project: Tika
> Issue Type: Task
> Reporter: Tim Allison
> Priority: Minor
>
> In 3x we had four different ways to fork parsing, and we needed to test all
> of them. Now that we have just the pipes option, we should be able to reduce
> oom/timeout tests.
>
> We should also be able to reduce startups of forked clients and tika-servers
> with a bit of thought.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
