Copilot commented on code in PR #3081:
URL: https://github.com/apache/tika/pull/3081#discussion_r3874566985
##########
tika-pipes/tika-pipes-core/src/main/java/org/apache/tika/pipes/core/ServerManager.java:
##########
@@ -95,61 +95,28 @@ public interface ServerManager extends Closeable {
*/
java.nio.file.Path getTempDirectory();
- /**
- * Marks the server for restart due to a fatal error (OOM, timeout, etc.).
- * <p>
- * This is called by clients when they receive a fatal error status from
the server.
- * It signals that the server process is stopping, even if {@link
#isRunning()}
- * might still return true briefly. The next call to {@link
#ensureRunning()} will
- * wait for the process to fully exit and then restart.
- * <p>
- * The reason form below defaults to this one, so this must NOT default to
the reason form:
- * an implementation overriding neither would recurse until the stack
blew. Concrete managers
- * in tika-pipes override both, so callers of either spelling reach a real
implementation.
- */
- default void markServerForRestart() {
- // Default no-op: preserves implementations written before
RestartReason existed.
- }
-
- /** As {@link #markServerForRestart()}, attributing the restart to {@code
reason}. Override this one. */
- default void markServerForRestart(RestartReason reason) {
- markServerForRestart();
- }
-
/**
* The generation of the currently running process: a counter incremented
every time this
* manager forks a replacement. A client captures it when it connects and
hands it back with
* every report, so a report about a process that has already been
replaced can be recognised
* and dropped rather than being applied to its healthy successor.
*/
- default long getGeneration() {
- return 0;
- }
-
- /**
- * As {@link #markServerForRestart(RestartReason)}, but only if {@code
generation} is still
- * current. Reports about a superseded process are dropped.
- */
- default void markServerForRestart(RestartReason reason, long generation) {
- markServerForRestart(reason);
- }
+ long getGeneration();
/**
- * The reasonless spelling of the above, kept for callers that cannot
attribute the failure.
- * Routed through the reason form rather than the bare no-arg default:
that default exists
- * only to keep pre-RestartReason implementations working, and delegating
here would leave
- * this silently inert for any implementation that overrides only the
reason form.
- */
- default void markServerForRestart(long generation) {
- markServerForRestart(RestartReason.CRASH, generation);
- }
-
- /**
- * As {@link #handleCrashAndGetExitCode()}, but only if {@code generation}
is still current.
+ * Marks the server for restart due to a fatal error, attributed to {@code
reason}, but only
+ * if {@code generation} is still current -- reports about a superseded
process are dropped.
+ * <p>
+ * Called by a client that received a fatal status: the process is
stopping even if
+ * {@link #isRunning()} still says otherwise, and the next {@link
#ensureRunning()} waits for
+ * it to exit and restarts it.
+ * <p>
+ * Deliberately the only spelling, and deliberately abstract. Earlier
revisions offered a
+ * no-arg and a reasonless form defaulting to one another; an
implementation that overrode
+ * only one left the others silently inert, which is how a worker known to
be poisoned kept
+ * being handed documents.
*/
- default int handleCrashAndGetExitCode(long generation) {
- return handleCrashAndGetExitCode();
- }
+ void markServerForRestart(RestartReason reason, long generation);
Review Comment:
Making these methods abstract (and removing the previous default/overload
spellings) is a source/binary breaking change for downstream `ServerManager`
implementations outside this repo. If this interface is part of Tika’s public
API surface, consider keeping the old methods as `@Deprecated` defaults for at
least one major/minor release (forwarding into the generation-aware methods),
or introduce a new sub-interface (e.g., `GenerationAwareServerManager`) to
preserve compatibility while moving callers over.
##########
tika-pipes/tika-pipes-core/src/main/java/org/apache/tika/pipes/core/ServerManager.java:
##########
@@ -205,9 +172,6 @@ default boolean needsRestart() {
*
* @return the exit code if available, or -1 if the process is still
running or unavailable
*/
- default int handleCrashAndGetExitCode() {
- markServerForRestart(RestartReason.CRASH);
- return -1;
- }
+ int handleCrashAndGetExitCode(long generation);
Review Comment:
Making these methods abstract (and removing the previous default/overload
spellings) is a source/binary breaking change for downstream `ServerManager`
implementations outside this repo. If this interface is part of Tika’s public
API surface, consider keeping the old methods as `@Deprecated` defaults for at
least one major/minor release (forwarding into the generation-aware methods),
or introduce a new sub-interface (e.g., `GenerationAwareServerManager`) to
preserve compatibility while moving callers over.
##########
tika-pipes/tika-pipes-core/src/test/java/org/apache/tika/pipes/core/PipesClientClosedManagerTest.java:
##########
@@ -0,0 +1,64 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.tika.pipes.core;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertNull;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+import java.net.ServerSocket;
+
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.Timeout;
+
+import org.apache.tika.metadata.Metadata;
+import org.apache.tika.parser.ParseContext;
+import org.apache.tika.pipes.api.FetchEmitTuple;
+import org.apache.tika.pipes.api.PipesResult;
+import org.apache.tika.pipes.api.emitter.EmitKey;
+import org.apache.tika.pipes.api.fetcher.FetchKey;
+
+public class PipesClientClosedManagerTest {
+
+ /**
+ * A request that reaches initialization after its manager was closed (a
parse racing
+ * PipesParser.close()/AsyncProcessor.close()) must come back as
FAILED_TO_INITIALIZE
+ * rather than escaping as an unchecked IllegalStateException -- and must
not mark a
+ * worker for restart, since there is nothing left to restart.
+ */
+ @Test
+ @Timeout(30)
+ public void closedManagerDuringInitReturnsFailedToInitialize() throws
Exception {
+ try (ServerSocket serverSocket = new ServerSocket(0)) {
+ SentinelServerManager manager = new
SentinelServerManager(serverSocket.getLocalPort());
+ manager.closed = true;
Review Comment:
The test sets `manager.closed` directly, which couples it to
`SentinelServerManager` internals. Prefer using the public `close()` method (or
a helper on the sentinel) to put the manager into the closed state; this keeps
the test resilient if the sentinel implementation details change.
##########
CHANGES.txt:
##########
@@ -1,78 +1,176 @@
Release 4.1.0 - unreleased
- * PDF: extractFontNames threw NullPointerException on a page with no
- /Resources dictionary (TIKA-4842).
-
- * tika-server: opt-in Micrometer metrics reporting and endpoint
- (TIKA-4839).
-
- * Per-request (parse-context) config for parsers that lock fields
- (Tess4J, VLM, OpenAI image-embedding) threw even when empty; locked
- fields are still rejected when actually set (TIKA-4843).
-
- * OOXML: new msoffice:has-unreferenced-parts and
- msoffice:unreferenced-part-names flag package parts unreachable via the
- OPC relationship graph. Structural only, expect false positives; not
- applied to XPS (TIKA-4837).
-
- * Shared pipes server (useSharedServer: true): a worker death could trigger
- a second, spurious restart that killed the healthy replacement. Forks now
- carry a generation; stale reports are dropped. Also fixed: a fork after
- shutdown() that was never destroyed, and a temp-dir leak on interrupt
- during teardown (TIKA-4844).
-
- * tika-pipes cache memory budget defaults to a quarter of the fork heap;
- override with -Dtika.pipes.cacheMemoryBudgetBytes in forkedJvmArgs
- (<=0 disables). TikaInputStream: hasFile() also reports cache spills,
- toString() no longer spills, new inMemoryContent(channel). Digester gains
- digestSink(); a digest is published only on commit(), so failed or empty
- translations (e.g. stub PST items) publish no digest. New
- TemporaryResources.closeAll(Closeable...) (TIKA-4835).
-
- * Docs/javadocs reconciled with the code: ES/OpenSearch attachmentStrategy
- has no default; Kafka connectionsMaxIdleMs is honored; jdbc
- queryTimeoutSeconds 0 is not "no limit"; Solr basic auth only; pipes
- reporters/iterators are built at config load; Tess4J also locks poolSize
- and maxImagePixels; pdf:trapped is new, not renamed; plugin config
- nesting fixed in 23 javadocs (TIKA-4842).
-
- * Pipes plugins no longer bundle Jackson; the host provides it. Plugin
- config is parsed by a shared strict PluginJson mapper (rejects unknown
- and duplicate keys; accepts comments) (TIKA-4840).
-
- * tika-server and tika-async-cli accept // and /* */ comments in config
- during override merging, as documented (TIKA-4834).
-
- * Kafka pipes iterator no longer stops on the first empty poll; waits for
- partition assignment (assignmentTimeoutMs, 30s) and a quiet window
- (drainIdleMs, 1s). groupInitialRebalanceDelayMs is deprecated
- (TIKA-4833).
-
- * Pipes IPC carries inline bytes as a raw binary field, not in the tuple;
- Smile 7-bit binary encoding disabled. 4.0.0 tuples with an "inline-bytes"
- parse-context entry are rejected (TIKA-4829).
-
- * Digesting embedded documents no longer spools each to a temp file; a
- process-wide CacheMemoryBudget (default 256MB) keeps them in memory. New
- TikaInputStream API: get(IOSupplier,...), enableRewind(CacheMemoryBudget),
- getSeekableByteChannel(). Zip-family parsing and detection use seekable
- channels, so hasFile() may be false afterward; getPath() still spools on
- demand (TIKA-4828).
-
- * Pipes carries the client Content-Type into the forked worker as a
- detection hint for all forked endpoints; honored only when it equals or
- specializes the detected type, or when there is no magic. The
- user-override key is not carried (TIKA-4825).
-
- * OneNote: document-order extraction, superseded revisions omitted, embedded
- BLOBs extracted, warnings and relationship IDs in metadata, bounded
- recursion/allocation; malformed files fall back to the legacy string dump
+ * tika-pipes: a parse racing PipesParser.close() now returns
FAILED_TO_INITIALIZE
+ instead of throwing IllegalStateException. ServerManager restart
reporting is
+ now the single abstract pair markServerForRestart(RestartReason, long) and
Review Comment:
This PR is titled \"TIKA-4839 - simplify signature\", but `CHANGES.txt` is
updated with a large batch of unrelated release notes (multiple
tickets/features). If those entries weren’t intended to be part of this change,
it would be better to limit `CHANGES.txt` updates to items directly related to
this PR (or move the broader changelog update into a separate PR) to keep
review scope and release attribution clear.
##########
tika-pipes/tika-pipes-core/src/main/java/org/apache/tika/pipes/core/PipesClient.java:
##########
@@ -235,6 +235,16 @@ public PipesResult process(FetchEmitTuple t) throws
IOException, InterruptedExce
closeConnection();
return buildFatalResult(t.getId(), t.getEmitKey(),
PipesResult.RESULT_STATUS.FAILED_TO_INITIALIZE,
intermediateResult.get());
+ } catch (IllegalStateException e) {
+ // Typically the manager was closed underneath us: a request
thread racing PipesParser.close()
+ // or AsyncProcessor.close(), which interrupts workers without
awaiting them. Nothing
+ // to restart and nothing to recover -- but report it rather than
letting an unchecked
+ // exception escape PipesParser.parse() to a caller that cannot
act on it.
+ LOG.warn("clientId={}: server manager rejected initialization of
{}", pipesClientId,
+ t.getId(), e);
+ closeConnection();
+ return buildFatalResult(t.getId(), t.getEmitKey(),
PipesResult.RESULT_STATUS.FAILED_TO_INITIALIZE,
+ intermediateResult.get(), e.getMessage());
}
Review Comment:
Catching a broad `IllegalStateException` here can accidentally convert
unrelated programming errors into `FAILED_TO_INITIALIZE`, making real bugs
harder to detect. If the intent is specifically \"manager was closed\" (or
similar lifecycle state), consider throwing/catching a more specific exception
from the manager (or checking a concrete condition/state) so truly unexpected
`IllegalStateException`s still fail fast and remain visible.
--
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]
