Don't close long-lived RowSets in Downsampler BucketState (#5478)

Fixes a bug in downsampling where a table may cause an error if freed while processing an update. Fixes #5476
deephaven · May 17, 2024 · 69378ea · 69378ea
1 parent 25d097a
commit 69378ea
Show file tree

Hide file tree

Showing 2 changed files with 7 additions and 16 deletions.
diff --git a/ClientSupport/src/main/java/io/deephaven/clientsupport/plotdownsampling/BucketState.java b/ClientSupport/src/main/java/io/deephaven/clientsupport/plotdownsampling/BucketState.java
@@ -9,6 +9,8 @@
 import io.deephaven.engine.rowset.RowSetFactory;
 import io.deephaven.engine.rowset.impl.RowSetUtils;
 import io.deephaven.engine.rowset.chunkattributes.OrderedRowKeys;
+import io.deephaven.internal.log.LoggerFactory;
+import io.deephaven.io.logger.Logger;
 import io.deephaven.util.QueryConstants;
 import io.deephaven.chunk.Chunk;
 import io.deephaven.chunk.LongChunk;
@@ -26,6 +28,8 @@
  * its own offset in those arrays.
  */
 public class BucketState {
+    private static final Logger log = LoggerFactory.getLogger(BucketState.class);
+
     private final WritableRowSet rowSet = RowSetFactory.empty();
 
     private RowSet cachedRowSet;
@@ -310,22 +314,16 @@ public void validate(final boolean usePrev, final DownsampleChunkContext context
                         values[columnIndex].validate(offset, keyChunk.get(indexInChunk), valueChunks[columnIndex],
                                 indexInChunk, trackNulls ? nulls[columnIndex] : null);
                     } catch (final RuntimeException e) {
-                        System.out.println(rowSet);
                         final String msg =
                                 "Bad data! indexInChunk=" + indexInChunk + ", col=" + columnIndex + ", usePrev="
-                                        + usePrev + ", offset=" + offset + ", rowSet=" + keyChunk.get(indexInChunk);
+                                        + usePrev + ", offset=" + offset + ", indexInChunk="
+                                        + keyChunk.get(indexInChunk);
+                        log.error().append(msg).append(", rowSet=").append(rowSet).endl();
                         throw new IllegalStateException(msg, e);
                     }
                 }
             }
         }
         Assert.eqTrue(makeRowSet().subsetOf(rowSet), "makeRowSet().subsetOf(rowSet)");
     }
-
-    public void close() {
-        if (cachedRowSet != null) {
-            cachedRowSet.close();
-        }
-        rowSet.close();
-    }
 }
diff --git a/...Support/src/main/java/io/deephaven/clientsupport/plotdownsampling/RunChartDownsample.java b/...Support/src/main/java/io/deephaven/clientsupport/plotdownsampling/RunChartDownsample.java
@@ -317,12 +317,6 @@ private DownsamplerListener(
             allYColumnIndexes = IntStream.range(0, key.yColumnNames.length).toArray();
         }
 
-        @Override
-        protected void destroy() {
-            super.destroy();
-            states.values().forEach(BucketState::close);
-        }
-
         @Override
         public void onUpdate(final TableUpdate upstream) {
             try (final DownsampleChunkContext context =
@@ -684,7 +678,6 @@ private void performRescans(final DownsampleChunkContext context) {
                     // if it has no keys at all, remove it so we quit checking it
                     iterator.remove();
                     releasePosition(bucket.getOffset());
-                    bucket.close();
                 } else {
                     bucket.rescanIfNeeded(context);
                 }