jerryshao commented on code in PR #13067:
URL: https://github.com/apache/gravitino/pull/13067#discussion_r3976709737
##########
server-common/src/main/java/org/apache/gravitino/server/web/JettyServer.java:
##########
@@ -484,6 +484,7 @@ public Thread run() {
thread.setName(getName() + "-" + thread.getId());
thread.setUncaughtExceptionHandler(
(t, throwable) -> {
+ ServerHealth.getInstance().recordFailure(throwable);
Review Comment:
In practice this handler never runs for errors thrown while handling a
request, so an OOM raised in a servlet filter is never recorded.
Jetty 9.4 catches `Throwable` before anything can reach a worker thread's
`UncaughtExceptionHandler`:
- `HttpChannel.handle` has `catch (Throwable failure)`, which calls
`handleException(failure)`, logs it, and sends a 500
(`HttpChannel.java:646-651`).
- `QueuedThreadPool`'s runner also catches `Throwable` around `runJob` and
only logs it (`QueuedThreadPool.java:1044-1047`).
So this line only fires if Jetty's own cleanup or logging code throws.
The Jersey `OutOfMemoryErrorListener` can't close that gap. Servlet filters
run before the Jersey `ServletContainer`, so an OOM there never reaches
`ON_EXCEPTION`.
A concrete case with authentication enabled: the heap is exhausted and an
`OutOfMemoryError` is thrown while an authenticator runs inside
`AuthenticationFilter`.
- A direct OOM isn't caught by the filter's `catch (Exception e)`. It
reaches Jetty's `Throwable` catch and becomes a 500, and is never recorded.
- A wrapped OOM (for example `RuntimeException(OOM)`) is caught at
`AuthenticationFilter.java:146-148` and turned into a response by
`sendAuthErrorResponse`, also without being recorded.
`RequestContextFilter`, `VersioningFilter`, the custom filters, and servlets
outside Jersey behave the same way. Every request can fail while
`/api/health/live` keeps returning 200. That is the "degraded but still
reported healthy" state this PR sets out to fix.
`TestJettyServer.testUncaughtOutOfMemoryUpdatesHealth` calls
`getUncaughtExceptionHandler().uncaughtException(...)` directly, so it doesn't
show that this path can actually be reached.
`docs/health-and-readiness.md:70-71` also lists "Jetty worker
uncaught-exception handlers" as a place where OOM gets recorded.
Suggestion: add a small servlet `Filter` installed first in the chain that
catches `Throwable`, records it, and rethrows. That covers every filter and
servlet behind it. A Jetty `ErrorHandler` would also work. Either is more
reliable than the thread handler.
##########
server-common/src/main/java/org/apache/gravitino/server/web/ServerHealth.java:
##########
@@ -0,0 +1,80 @@
+/*
+ * 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.gravitino.server.web;
+
+import javax.annotation.Nullable;
+
+/**
+ * Records out-of-memory failures observed by the server. Once recorded, the
failure remains until
+ * the process restarts; a successful request does not establish that the JVM
has recovered.
+ *
+ * <p>The failure path only sets a flag. It does not retain the error, format
its stack trace, or
+ * allocate a collection while walking its causes.
+ */
+public final class ServerHealth {
+ private static final ServerHealth INSTANCE = new ServerHealth();
+
+ private volatile boolean outOfMemory;
+
+ /** Creates an independent health state, initially healthy. */
+ public ServerHealth() {}
+
+ /**
+ * Returns the shared state used by the server's request and health-check
paths.
+ *
+ * @return the shared health state
+ */
+ public static ServerHealth getInstance() {
+ return INSTANCE;
+ }
+
+ /**
+ * Records an out-of-memory error, including one wrapped in another
throwable.
+ *
+ * @param failure the observed failure, or null
+ */
+ public void recordFailure(@Nullable Throwable failure) {
Review Comment:
Minor gap, lower impact than the direct and wrapped cases: this walk follows
only `getCause()`, so an `OutOfMemoryError` that exists only as a *suppressed*
exception is missed at every boundary.
Scenario: code uses try-with-resources. The body throws an `IOException`,
then the resource's `close()` throws `OutOfMemoryError` because the heap is
exhausted. Java attaches the OOM to the `IOException` with `addSuppressed`, not
as its cause. When that exception, or a `RuntimeException` wrapping it, reaches
`Utils.doAs`, `Utils.internalError`, the Iceberg/Lance `toRESTResponse`, or the
Jersey listener, this method finds no OOM in the cause chain. The marker stays
`false` while the client gets a 500.
To cover it, also check each visited node's `getSuppressed()` one level
deep. One caveat: `getSuppressed()` returns a defensive array copy, so this
conflicts with the "no allocation on the failure path" goal. Accepting a small
allocation here, or documenting the limitation, both seem fine to me.
I also verified the cycle detection itself, and it is correct. The fast
pointer checks every node before moving past it, so no OOM node is skipped, and
it terminates on self-causes, 2-cycles, and long cycles after a long prefix.
--
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]
