From 85415048fe69908bd678c45b064d4567a574956b Mon Sep 17 00:00:00 2001 From: Colin Alworth Date: Tue, 16 Jul 2024 19:12:07 -0500 Subject: [PATCH] refactor!: Remove Assert/Require methods that require locks/reflections Partial #188 --- .../java/io/deephaven/base/verify/Assert.java | 42 ------------------- .../multiseries/AbstractMultiSeries.java | 2 +- .../util/channel/CachedChannelProvider.java | 2 +- .../util/datastructures/SubscriptionSet.java | 2 +- .../engine/table/impl/SortOperation.java | 2 +- .../table/impl/by/AggregationProcessor.java | 2 +- .../table/impl/lang/QueryLanguageParser.java | 5 ++- .../impl/util/BaseArrayBackedInputTable.java | 2 +- .../table/impl/QueryTableAggregationTest.java | 2 +- .../engine/util/TestJobScheduler.java | 20 +++++---- .../barrage/table/BarrageTable.java | 2 +- .../barrage/BarrageMessageProducer.java | 8 ++-- .../HierarchicalTableViewSubscription.java | 4 +- 13 files changed, 28 insertions(+), 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 index 394eac6260b..130c2855e6f 100644 --- 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 && (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 && !(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 index b8bbbf46c80..5df283c53b1 100644 --- 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 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 index bb5bafda49b..fe040fd7770 100644 --- 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 index 0d9fc96a8c0..ccb114cfbe1 100644 --- 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 index 34f65b86f7a..29df1ace3fc 100644 --- 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 index f57ac5dae87..056af86db68 100644 --- 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 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 index 36b10f914fa..dca741d3fbe 100644 --- 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/engine/table/src/main/java/io/deephaven/engine/table/impl/util/BaseArrayBackedInputTable.java b/engine/table/src/main/java/io/deephaven/engine/table/impl/util/BaseArrayBackedInputTable.java index b1d09589f73..88f5097080f 100644 --- a/engine/table/src/main/java/io/deephaven/engine/table/impl/util/BaseArrayBackedInputTable.java +++ b/engine/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 index 09f0075f025..ce5fa9bbdf4 100644 --- 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 index 2e05d840473..3addddef5d0 100644 --- 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 index 118c968e479..514f76f05d6 100644 --- 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 index ddc067f0a33..def2a56a760 100644 --- 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/server/src/main/java/io/deephaven/server/hierarchicaltable/HierarchicalTableViewSubscription.java b/server/src/main/java/io/deephaven/server/hierarchicaltable/HierarchicalTableViewSubscription.java index 4bf7f7e52b4..a0e18a46ad6 100644 --- a/server/src/main/java/io/deephaven/server/hierarchicaltable/HierarchicalTableViewSubscription.java +++ b/server/src/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) {