Copilot commented on code in PR #3081:
URL: https://github.com/apache/tika/pull/3081#discussion_r3871500554
##########
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()` and `markServerForRestart(RestartReason, long)`
abstract (and removing older overloads) is a source/binary breaking change for
any downstream `ServerManager` implementations. If `ServerManager` is
considered part of a supported public API, consider keeping the older overloads
as `@Deprecated` for a transition period (even if they throw or delegate in a
documented best-effort way), or clearly documenting the breaking change in the
public API docs/release notes so downstreams know to update.
##########
tika-pipes/tika-pipes-core/src/main/java/org/apache/tika/pipes/core/PipesClient.java:
##########
@@ -235,6 +235,15 @@ 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());
Review Comment:
The warning log drops the exception, which makes diagnosing shutdown races
harder in production (you lose stack trace / root cause). Consider logging the
throwable (e.g., pass `e` as the last argument) so the WARN includes the
exception details.
##########
tika-pipes/tika-pipes-core/src/main/java/org/apache/tika/pipes/core/PipesClient.java:
##########
@@ -235,6 +235,15 @@ 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());
+ closeConnection();
+ return buildFatalResult(t.getId(), t.getEmitKey(),
PipesResult.RESULT_STATUS.FAILED_TO_INITIALIZE,
+ intermediateResult.get(), e.getMessage());
Review Comment:
The new behavior converting an initialization-time `IllegalStateException`
into a `FAILED_TO_INITIALIZE` `PipesResult` looks user-visible and should be
pinned by a unit/integration test (e.g., closing the manager while a client is
initializing) to prevent regressions back to an unchecked exception escaping
`PipesParser.parse()`.
--
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]
