refactor!: Remove Assert/Require methods that require locks/reflections

Partial deephaven#188
niloc132 · Jul 17, 2024 · 8541504 · 8541504
1 parent 8f54141
commit 8541504
Show file tree

Hide file tree

Showing 13 changed files with 28 additions and 67 deletions.
diff --git a/Base/src/main/java/io/deephaven/base/verify/Assert.java b/Base/src/main/java/io/deephaven/base/verify/Assert.java
@@ -316,48 +316,6 @@ public static AssertionFailure valueNeverOccurs(double d, String name) {
         return null;
     }
 
-    // ################################################################
-    // holdsLock, notHoldsLock
-
-    // ----------------------------------------------------------------
-    /** assert (o != null &amp;&amp; (current thread holds o's lock)) */
-    public static void holdsLock(Object o, String name) {
-        neqNull(o, "o");
-        if (!Thread.holdsLock(o)) {
-            fail("\"" + Thread.currentThread().getName() + "\".holdsLock(" + name + ")");
-        }
-    }
-
-    // ----------------------------------------------------------------
-    /** assert (o != null &amp;&amp; !(current thread holds o's lock)) */
-    public static void notHoldsLock(Object o, String name) {
-        neqNull(o, "o");
-        if (Thread.holdsLock(o)) {
-            fail("!\"" + Thread.currentThread().getName() + "\".holdsLock(" + name + ")");
-        }
-    }
-
-    // ################################################################
-    // instanceOf, notInstanceOf
-
-    // ----------------------------------------------------------------
-    /** assert (o instanceof type) */
-    public static void instanceOf(Object o, String name, Class<?> type) {
-        if (!type.isInstance(o)) {
-            fail(name + " instanceof " + type, null == o ? ExceptionMessageUtil.valueAndName(o, name)
-                    : name + " instanceof " + o.getClass() + " (" + ExceptionMessageUtil.valueAndName(o, name) + ")");
-        }
-    }
-
-    // ----------------------------------------------------------------
-    /** assert !(o instanceof type) */
-    public static void notInstanceOf(Object o, String name, Class<?> type) {
-        if (type.isInstance(o)) {
-            fail("!(" + name + " instanceof " + type + ")",
-                    name + " instanceof " + o.getClass() + " (" + ExceptionMessageUtil.valueAndName(o, name) + ")");
-        }
-    }
-
     // ################################################################
     // eq (primitiveValue == primitiveValue)
 

diff --git a/Plot/src/main/java/io/deephaven/plot/datasets/multiseries/AbstractMultiSeries.java b/Plot/src/main/java/io/deephaven/plot/datasets/multiseries/AbstractMultiSeries.java
@@ -218,7 +218,7 @@ private List<SERIES> getSeries() {
     @Override
     // this should be run under a seriesLock
     public void addSeries(SERIES series, Object key) {
-        Assert.holdsLock(seriesLock, "seriesLock");
+        Assert.assertion(Thread.holdsLock(seriesLock), "Thread.holdsLock(seriesLock)");
 
         if (seriesKeys == null) {
             seriesKeys = new HashSet<>();

diff --git a/Util/channel/src/main/java/io/deephaven/util/channel/CachedChannelProvider.java b/Util/channel/src/main/java/io/deephaven/util/channel/CachedChannelProvider.java
@@ -168,7 +168,7 @@ private synchronized void returnPoolableChannel(@NotNull final CachedChannel cac
     }
 
     private long advanceClock() {
-        Assert.holdsLock(this, "this");
+        Assert.assertion(Thread.holdsLock(this), "Thread.holdsLock(this)");
         final long newClock = ++logicalClock;
         if (newClock > 0) {
             return newClock;

diff --git a/Util/src/main/java/io/deephaven/util/datastructures/SubscriptionSet.java b/Util/src/main/java/io/deephaven/util/datastructures/SubscriptionSet.java
@@ -54,7 +54,7 @@ private boolean isActive() {
          * Activate this subscription entry. Must hold the lock on the enclosing subscription set.
          */
         public void activate() {
-            Assert.holdsLock(SubscriptionSet.this, "SubscriptionSet.this");
+            Assert.assertion(Thread.holdsLock(SubscriptionSet.this), "Thread.holdsLock(SubscriptionSet.this)");
             active = true;
         }
     }

diff --git a/engine/table/src/main/java/io/deephaven/engine/table/impl/SortOperation.java b/engine/table/src/main/java/io/deephaven/engine/table/impl/SortOperation.java
@@ -362,7 +362,7 @@ public static LongUnaryOperator getReverseLookup(@NotNull final Table parent, @N
             Assert.neqNull(value, "sort result reverse lookup");
         }
         if (value != null) {
-            Assert.instanceOf(value, "sort result reverse lookup", LongUnaryOperator.class);
+            Assert.assertion(value instanceof LongUnaryOperator, "sort result reverse lookup");
             return (LongUnaryOperator) value;
         }
         final RowRedirection sortRedirection = getRowRedirection(sortResult);

diff --git a/engine/table/src/main/java/io/deephaven/engine/table/impl/by/AggregationProcessor.java b/engine/table/src/main/java/io/deephaven/engine/table/impl/by/AggregationProcessor.java
@@ -2005,7 +2005,7 @@ public void supplyRowLookup(@NotNull final Supplier<AggregationRowLookup> rowLoo
     public static AggregationRowLookup getRowLookup(@NotNull final Table aggregationResult) {
         final Object value = aggregationResult.getAttribute(AGGREGATION_ROW_LOOKUP_ATTRIBUTE);
         Assert.neqNull(value, "aggregation result row lookup");
-        Assert.instanceOf(value, "aggregation result row lookup", AggregationRowLookup.class);
+        Assert.assertion(value instanceof AggregationRowLookup, "aggregation result row lookup");
         return (AggregationRowLookup) value;
     }
 }
diff --git a/engine/table/src/main/java/io/deephaven/engine/table/impl/lang/QueryLanguageParser.java b/engine/table/src/main/java/io/deephaven/engine/table/impl/lang/QueryLanguageParser.java
@@ -1670,8 +1670,9 @@ public Class<?> visit(UnaryExpr n, VisitArgs printer) {
             if (isNonequalOpOverload && printer.hasStringBuilder()) {
                 // sanity checks -- the inner expression *must* be a BinaryExpr (for ==), and it must be replaced in
                 // this UnaryExpr with a MethodCallExpr (for "eq()" or possibly "isNull()").
-                Assert.instanceOf(n.getExpression(), "n.getExpression()", MethodCallExpr.class);
-                final MethodCallExpr methodCall = (MethodCallExpr) n.getExpression();
+                Object o = n.getExpression();
+                Assert.assertion(o instanceof MethodCallExpr, "n.getExpression()");
+                final MethodCallExpr methodCall = (MethodCallExpr) o;
                 final String methodName = methodCall.getNameAsString();
                 if (!"eq".equals(methodName) && !"isNull".equals(methodName)) {
                     throw new IllegalStateException(

diff --git a/...ne/table/src/main/java/io/deephaven/engine/table/impl/util/BaseArrayBackedInputTable.java b/...ne/table/src/main/java/io/deephaven/engine/table/impl/util/BaseArrayBackedInputTable.java
@@ -171,7 +171,7 @@ private final class PendingChange {
         String error;
 
         private PendingChange(@NotNull Table table, boolean delete) {
-            Assert.holdsLock(pendingChanges, "pendingChanges");
+            Assert.assertion(Thread.holdsLock(pendingChanges), "Thread.holdsLock(pendingChanges)");
             Assert.neqNull(table, "table");
             this.table = table;
             this.delete = delete;

diff --git a/engine/table/src/test/java/io/deephaven/engine/table/impl/QueryTableAggregationTest.java b/engine/table/src/test/java/io/deephaven/engine/table/impl/QueryTableAggregationTest.java
@@ -3921,7 +3921,7 @@ public void testKeyColumnMissing() {
             final Table agg = data.selectDistinct("NonExistentCol");
             fail("Should have thrown an exception");
         } catch (Exception ex) {
-            io.deephaven.base.verify.Assert.instanceOf(ex, "ex", IllegalArgumentException.class);
+            assertTrue(ex instanceof IllegalArgumentException);
             io.deephaven.base.verify.Assert.assertion(
                     ex.getMessage().contains("Missing columns: [NonExistentCol]"),
                     "ex.getMessage().contains(\"Missing columns: [NonExistentCol]\")",

diff --git a/engine/table/src/test/java/io/deephaven/engine/util/TestJobScheduler.java b/engine/table/src/test/java/io/deephaven/engine/util/TestJobScheduler.java
@@ -23,6 +23,8 @@
 import java.util.concurrent.ExecutionException;
 import java.util.concurrent.atomic.AtomicInteger;
 
+import static org.junit.Assert.assertNotNull;
+
 public final class TestJobScheduler {
 
     @Rule
@@ -149,7 +151,7 @@ public void close() {
                     50,
                     (context, idx, nec, resume) -> {
                         // verify the type is correct
-                        Assert.instanceOf(context, "context", TestJobThreadContext.class);
+                        assertNotNull(context);
 
                         completed[idx] = true;
                         resume.run();
@@ -262,7 +264,7 @@ public void close() {
                     50,
                     (context, idx, nec, resume) -> {
                         // verify the type is correct
-                        Assert.instanceOf(context, "context", TestJobThreadContext.class);
+                        assertNotNull(context);
 
                         completed[idx] = true;
                         resume.run();
@@ -396,7 +398,7 @@ public void close() {
                     50,
                     (context, idx, nec) -> {
                         // verify the type is correct
-                        Assert.instanceOf(context, "context", TestJobThreadContext.class);
+                        assertNotNull(context);
 
                         // throw before "doing work" to make verification easy
                         if (idx == 10) {
@@ -467,7 +469,7 @@ public void close() {
                     50,
                     (context, idx, nec, resume) -> {
                         // verify the type is correct
-                        Assert.instanceOf(context, "context", TestJobThreadContext.class);
+                        assertNotNull(context);
 
                         completed[idx] = true;
 
@@ -552,7 +554,7 @@ public void close() {
                                 60,
                                 (context2, idx2, nec2) -> {
                                     // verify the type is correct
-                                    Assert.instanceOf(context2, "context2", TestJobThreadContext.class);
+                                    assertNotNull(context2);
 
                                     // throw before "doing work" to make verification easy
                                     if (idx1 == 10 && idx2 == 10) {
@@ -632,7 +634,7 @@ public void close() {
                                 60,
                                 (context2, idx2, nec2) -> {
                                     // verify the type is correct
-                                    Assert.instanceOf(context2, "context2", TestJobThreadContext.class);
+                                    assertNotNull(context2);
                                     completed[idx1][idx2] = true;
                                 }, r1, nec1);
                     },
@@ -702,7 +704,7 @@ public void close() {
                                 60,
                                 (context2, idx2, nec2) -> {
                                     // verify the type is correct
-                                    Assert.instanceOf(context2, "context2", TestJobThreadContext.class);
+                                    assertNotNull(context2);
                                     completed[idx1][idx2] = true;
                                 }, r1, nec1);
                     },
@@ -770,7 +772,7 @@ public void close() {
                         50,
                         (context, idx, nec) -> {
                             // verify the type is correct
-                            Assert.instanceOf(context, "context", TestJobThreadContext.class);
+                            assertNotNull(context);
 
                             // throw before "doing work" to make verification easy
                             if (idx == 10) {
@@ -825,7 +827,7 @@ public void close() {
                         50,
                         (context, idx, nec) -> {
                             // verify the type is correct
-                            Assert.instanceOf(context, "context", TestJobThreadContext.class);
+                            assertNotNull(context);
                             completed[idx] = true;
                         },
                         () -> {

diff --git a/extensions/barrage/src/main/java/io/deephaven/extensions/barrage/table/BarrageTable.java b/extensions/barrage/src/main/java/io/deephaven/extensions/barrage/table/BarrageTable.java
@@ -293,7 +293,7 @@ protected synchronized void updateServerViewport(
             final RowSet viewport,
             final BitSet columns,
             final boolean reverseViewport) {
-        Assert.holdsLock(this, "BarrageTable.this");
+        Assert.assertion(Thread.holdsLock(this), "Thread.holdsLock(this)");
 
         final RowSet finalViewport = viewport == null ? null : viewport.copy();
         final BitSet finalColumns = (columns == null || columns.cardinality() == numColumns())

diff --git a/server/src/main/java/io/deephaven/server/barrage/BarrageMessageProducer.java b/server/src/main/java/io/deephaven/server/barrage/BarrageMessageProducer.java
@@ -695,7 +695,7 @@ public void close() {
     }
 
     private void enqueueUpdate(final TableUpdate upstream) {
-        Assert.holdsLock(this, "enqueueUpdate must hold lock!");
+        Assert.assertion(Thread.holdsLock(this), "Thread.holdsLock(this)");
 
         final WritableRowSet addsToRecord;
         final RowSet modsToRecord;
@@ -952,7 +952,7 @@ private void enqueueUpdate(final TableUpdate upstream) {
     }
 
     private void schedulePropagation() {
-        Assert.holdsLock(this, "schedulePropagation must hold lock!");
+        Assert.assertion(Thread.holdsLock(this), "schedulePropagation must hold lock!");
 
         // copy lastUpdateTime so we are not duped by the re-read
         final long localLastUpdateTime = lastUpdateTime;
@@ -1625,7 +1625,7 @@ private void propagateSnapshotForSubscription(final Subscription subscription,
     }
 
     private BarrageMessage aggregateUpdatesInRange(final int startDelta, final int endDelta) {
-        Assert.holdsLock(this, "propagateUpdatesInRange must hold lock!");
+        Assert.assertion(Thread.holdsLock(this), "propagateUpdatesInRange must hold lock!");
 
         final boolean singleDelta = endDelta - startDelta == 1;
         final BarrageMessage downstream = new BarrageMessage();
@@ -2110,7 +2110,7 @@ private void buildPostSnapshotViewports(boolean ignorePending) {
     }
 
     private void promoteSnapshotToActive() {
-        Assert.holdsLock(this, "promoteSnapshotToActive must hold lock!");
+        Assert.assertion(Thread.holdsLock(this), "Thread.holdsLock(this)");
 
         if (activeViewport != null) {
             activeViewport.close();

diff --git a/...rc/main/java/io/deephaven/server/hierarchicaltable/HierarchicalTableViewSubscription.java b/...rc/main/java/io/deephaven/server/hierarchicaltable/HierarchicalTableViewSubscription.java
@@ -422,7 +422,7 @@ public void setViewport(
     }
 
     private void scheduleImmediately(final long currentTimeNanos) {
-        Assert.holdsLock(schedulingLock, "schedulingLock");
+        Assert.assertion(Thread.holdsLock(schedulingLock), "Thread.holdsLock(schedulingLock)");
         if (!snapshotPending || currentTimeNanos < scheduledTimeNanos) {
             snapshotPending = true;
             scheduledTimeNanos = currentTimeNanos;
@@ -431,7 +431,7 @@ private void scheduleImmediately(final long currentTimeNanos) {
     }
 
     private void scheduleAtInterval(final long currentTimeNanos) {
-        Assert.holdsLock(schedulingLock, "schedulingLock");
+        Assert.assertion(Thread.holdsLock(schedulingLock), "Thread.holdsLock(schedulingLock)");
         final long targetTimeNanos = lastSnapshotTimeNanos + intervalDurationNanos;
         final long delayNanos = targetTimeNanos - currentTimeNanos;
         if (delayNanos < 0) {