diff --git a/src/java/org/apache/cassandra/db/virtual/ExceptionsTable.java b/src/java/org/apache/cassandra/db/virtual/ExceptionsTable.java index e3055c4c2ee6..5ecc0a618d8f 100644 --- a/src/java/org/apache/cassandra/db/virtual/ExceptionsTable.java +++ b/src/java/org/apache/cassandra/db/virtual/ExceptionsTable.java @@ -52,6 +52,14 @@ public class ExceptionsTable extends AbstractMutableVirtualTable */ static final List preInitialisationBuffer = Collections.synchronizedList(new ArrayList<>()); + /** + * Upper bound on {@link #preInitialisationBuffer}. Bounds heap retention if uncaught exceptions storm during early + * startup (before virtual tables are registered), or in offline/tool contexts that never register virtual tables + * and thus never call {@link #flush()}. Kept in line with the live buffer's default cap. + */ + @VisibleForTesting + static final int PRE_INITIALISATION_BUFFER_CAPACITY = 1000; + @VisibleForTesting static volatile ExceptionsTable INSTANCE; @@ -62,7 +70,7 @@ public class ExceptionsTable extends AbstractMutableVirtualTable ExceptionsTable(String keyspace) { // for starters capped to 1k, I do not think we need to make this configurable (yet). - this(keyspace, 1000); + this(keyspace, PRE_INITIALISATION_BUFFER_CAPACITY); } ExceptionsTable(String keyspace, int maxSize) @@ -84,10 +92,19 @@ public class ExceptionsTable extends AbstractMutableVirtualTable public void flush() { - for (ExceptionRow row : preInitialisationBuffer) - add(row.exceptionClass, row.exceptionLocation, row.message, row.stackTrace, row.occurrence.getTime()); + // Drain under the list's monitor and iterate a private copy: preInitialisationBuffer is a synchronizedList, + // whose contract requires holding its monitor while iterating. A concurrent persist() on another thread could + // otherwise add() during iteration and trigger a ConcurrentModificationException, which would propagate out of + // setupVirtualKeyspaces() and abort node startup. + List drained; + synchronized (preInitialisationBuffer) + { + drained = new ArrayList<>(preInitialisationBuffer); + preInitialisationBuffer.clear(); + } - preInitialisationBuffer.clear(); + for (ExceptionRow row : drained) + add(row.exceptionClass, row.exceptionLocation, row.message, row.stackTrace, row.occurrence.getTime()); } @Override @@ -178,12 +195,19 @@ public static void persist(Throwable t) } else { - preInitialisationBuffer.add(new ExceptionRow(toPersist.getClass().getName(), - stackTrace.isEmpty() ? "unknown" : stackTrace.get(0), - 0, - toPersist.getMessage(), - stackTrace, - now)); + // Bound retention (see PRE_INITIALISATION_BUFFER_CAPACITY): keep the earliest entries, which are usually the + // most diagnostic, and drop once full rather than growing without limit. Guard the size check and the add + // together under the list monitor so concurrent persist() calls cannot race past the cap. + synchronized (preInitialisationBuffer) + { + if (preInitialisationBuffer.size() < PRE_INITIALISATION_BUFFER_CAPACITY) + preInitialisationBuffer.add(new ExceptionRow(toPersist.getClass().getName(), + stackTrace.isEmpty() ? "unknown" : stackTrace.get(0), + 0, + toPersist.getMessage(), + stackTrace, + now)); + } } } diff --git a/src/java/org/apache/cassandra/utils/JVMStabilityInspector.java b/src/java/org/apache/cassandra/utils/JVMStabilityInspector.java index b11dafbc7294..37f5ef36002e 100644 --- a/src/java/org/apache/cassandra/utils/JVMStabilityInspector.java +++ b/src/java/org/apache/cassandra/utils/JVMStabilityInspector.java @@ -74,7 +74,9 @@ public static void uncaughtException(Thread thread, Throwable t) try { StorageMetrics.uncaughtExceptions.inc(); } catch (Throwable ignore) { /* might not be initialised */ } logger.error("Exception in thread {}", thread, t); Tracing.trace("Exception in thread {}", thread, t); - ExceptionsTable.persist(t); + // Recording the exception for observability must never preempt the stability handling below (the + // disk_failure_policy / OOM "die" actions in inspectThrowable). Guard it like the StorageMetrics increment above. + try { ExceptionsTable.persist(t); } catch (Throwable ignore) { /* observability only, must not throw here */ } for (Throwable t2 = t; t2 != null; t2 = t2.getCause()) { // make sure error gets logged exactly once. diff --git a/test/unit/org/apache/cassandra/db/virtual/ExceptionsTableTest.java b/test/unit/org/apache/cassandra/db/virtual/ExceptionsTableTest.java index e01c73f1064c..51140cb22021 100644 --- a/test/unit/org/apache/cassandra/db/virtual/ExceptionsTableTest.java +++ b/test/unit/org/apache/cassandra/db/virtual/ExceptionsTableTest.java @@ -287,6 +287,25 @@ public void testEmptyStacktrace() }); } + @Test + public void testPreInitialisationBufferIsBounded() + { + doWithVTable(100, table -> + { + // Do not register the table, so INSTANCE stays null and every persist() lands in the pre-initialisation + // buffer. Persisting well past the cap must not grow the buffer without bound. + ExceptionsTable.INSTANCE = null; + ExceptionsTable.preInitialisationBuffer.clear(); + + int overCap = ExceptionsTable.PRE_INITIALISATION_BUFFER_CAPACITY + 50; + for (int i = 0; i < overCap; i++) + ExceptionsTable.persist(new MyUncaughtException("boom " + i)); + + assertEquals(ExceptionsTable.PRE_INITIALISATION_BUFFER_CAPACITY, + ExceptionsTable.preInitialisationBuffer.size()); + }); + } + private List rows(String query) { return execute(query).stream().collect(toList());