Copilot commented on code in PR #3081:
URL: https://github.com/apache/tika/pull/3081#discussion_r3873288079
##########
tika-pipes/tika-pipes-core/src/test/java/org/apache/tika/pipes/core/SentinelServerManager.java:
##########
@@ -68,6 +74,21 @@ public Path getTempDirectory() {
return null;
}
+ @Override
+ public long getGeneration() {
+ return 0;
+ }
+
+ @Override
+ public void markServerForRestart(RestartReason reason, long generation) {
+ marked = reason;
+ }
+
+ @Override
+ public int handleCrashAndGetExitCode(long generation) {
+ return -1;
+ }
+
@Override
public void close() {
Review Comment:
`SentinelServerManager` introduces a `closed` flag and documents that
`ensureRunning()` should throw after close, but `close()` does not set `closed
= true`. This makes the helper behave differently depending on whether tests
set the flag directly vs. calling `close()`. Consider setting `closed = true`
inside `close()` (and keeping it idempotent) so the sentinel consistently
simulates a real closed manager.
##########
tika-pipes/tika-pipes-core/src/main/java/org/apache/tika/pipes/core/PerClientServerManager.java:
##########
@@ -291,18 +292,31 @@ public boolean needsRestart() {
return pendingRestart;
}
+ /**
+ * One client owns one manager here, so {@code generation} carries no
information a sibling
+ * could invalidate and is accepted only to satisfy the single {@link
ServerManager} spelling.
+ * Shared mode is where staleness is real.
+ */
@Override
- public void markServerForRestart() {
- markServerForRestart(RestartReason.CRASH);
+ public void markServerForRestart(RestartReason reason, long generation) {
Review Comment:
The parameter name `generation` here shadows the class field `generation`
and is intentionally unused in per-client mode. To avoid confusion, rename the
parameter to something like `ignoredGeneration` (or similar) to make it
explicit that it is not consulted.
##########
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) {
+ // 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 closed while initializing
{}", pipesClientId,
+ t.getId(), e);
+ closeConnection();
+ return buildFatalResult(t.getId(), t.getEmitKey(),
PipesResult.RESULT_STATUS.FAILED_TO_INITIALIZE,
+ intermediateResult.get(), e.getMessage());
Review Comment:
Catching `IllegalStateException` broadly can hide unrelated programming
errors during initialization (not only the 'manager was closed' scenario). If
feasible, consider narrowing this to a more specific exception type thrown by
`ServerManager` when closed (or checking a dedicated signal/exception message),
so unexpected `IllegalStateException`s still fail fast rather than being
converted into `FAILED_TO_INITIALIZE`.
##########
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 `getGeneration()`, `markServerForRestart(RestartReason, long)`, and
`handleCrashAndGetExitCode(long)` abstract is a source/binary breaking change
for any downstream `ServerManager` implementations. If maintaining
compatibility is a goal for this release line, consider keeping deprecated
default methods for one release (delegating to the new abstract methods), or
providing an adapter/base class to minimize downstream breakage.
##########
CHANGES.txt:
##########
@@ -1,5 +1,14 @@
Release 4.1.0 - unreleased
+ * tika-pipes: a parse that raced PipesParser.close()/AsyncProcessor.close()
+ threw IllegalStateException out of PipesParser.parse() in per-client mode;
+ both modes now return a FAILED_TO_INITIALIZE result, which a caller can
act
+ on. ServerManager's restart-reporting surface is reduced to one spelling
--
+ markServerForRestart(RestartReason, long) and
handleCrashAndGetExitCode(long),
+ both abstract. The previous no-arg and reasonless forms defaulted to one
+ another, so an implementation that overrode only one left the others
+ silently inert (TIKA-4839).
+
* Add Micrometer reporting and opt-in endpoint for tika-server (TIKA-4839).
Review Comment:
This adds a second CHANGES entry tagged `(TIKA-4839)` while the existing
entry for Micrometer also references `(TIKA-4839)`. If these are distinct
issues, one of the IDs should be corrected; if they are the same issue,
consider consolidating into a single bullet to avoid confusing release notes.
--
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]
