From de68812f0776c8d246d9fe8bde47a74c23ed7496 Mon Sep 17 00:00:00 2001 From: Colin Alworth Date: Thu, 5 Dec 2024 16:14:00 -0600 Subject: [PATCH] fix: JS subscriptions should return correct size while updating (#6463) Fixes #6423 --- .../io/deephaven/web/client/api/JsTable.java | 9 ++--- .../AbstractTableSubscription.java | 38 ++++++++++++++----- .../TableViewportSubscription.java | 6 +-- 3 files changed, 35 insertions(+), 18 deletions(-) diff --git a/web/client-api/src/main/java/io/deephaven/web/client/api/JsTable.java b/web/client-api/src/main/java/io/deephaven/web/client/api/JsTable.java index 04127edec70..9fb9e1c6c30 100644 --- a/web/client-api/src/main/java/io/deephaven/web/client/api/JsTable.java +++ b/web/client-api/src/main/java/io/deephaven/web/client/api/JsTable.java @@ -452,7 +452,7 @@ public JsLayoutHints getLayoutHints() { @JsProperty public double getSize() { TableViewportSubscription subscription = subscriptions.get(getHandle()); - if (subscription != null && subscription.getStatus() == TableViewportSubscription.Status.ACTIVE) { + if (subscription != null && subscription.hasValidSize()) { // only ask the viewport for the size if it is alive and ticking return subscription.size(); } @@ -705,7 +705,7 @@ public TableViewportSubscription setViewport(double firstRow, double lastRow, Column[] columnsCopy = columns != null ? Js.uncheckedCast(columns.slice()) : state().getColumns(); ClientTableState currentState = state(); TableViewportSubscription activeSubscription = subscriptions.get(getHandle()); - if (activeSubscription != null && activeSubscription.getStatus() != TableViewportSubscription.Status.DONE) { + if (activeSubscription != null && !activeSubscription.isClosed()) { // hasn't finished, lets reuse it activeSubscription.setInternalViewport(firstRow, lastRow, columnsCopy, updateIntervalMs, isReverseViewport); return activeSubscription; @@ -1583,8 +1583,7 @@ public void setState(final ClientTableState state) { if (!isClosed() && was != null && was != state()) { // if we held a subscription TableViewportSubscription existingSubscription = subscriptions.remove(was.getHandle()); - if (existingSubscription != null - && existingSubscription.getStatus() != TableViewportSubscription.Status.DONE) { + if (existingSubscription != null && !existingSubscription.isClosed()) { JsLog.debug("closing old viewport", state(), existingSubscription.state()); // with the replacement state successfully running, we can shut down the old viewport (unless // something external retained it) @@ -1715,7 +1714,7 @@ public void setSize(double s) { this.size = s; TableViewportSubscription subscription = subscriptions.get(getHandle()); - if (changed && (subscription == null || subscription.getStatus() == TableViewportSubscription.Status.DONE)) { + if (changed && (subscription == null || !subscription.hasValidSize())) { // If the size changed, and we have no subscription active, fire. Otherwise, we want to let the // subscription itself manage this, so that the size changes are synchronized with data changes, // and consumers won't be confused by the table size not matching data. diff --git a/web/client-api/src/main/java/io/deephaven/web/client/api/subscription/AbstractTableSubscription.java b/web/client-api/src/main/java/io/deephaven/web/client/api/subscription/AbstractTableSubscription.java index f231a93a071..efd0c593078 100644 --- a/web/client-api/src/main/java/io/deephaven/web/client/api/subscription/AbstractTableSubscription.java +++ b/web/client-api/src/main/java/io/deephaven/web/client/api/subscription/AbstractTableSubscription.java @@ -61,7 +61,7 @@ public abstract class AbstractTableSubscription extends HasEventHandling { */ public static final String EVENT_UPDATED = "updated"; - public enum Status { + protected enum Status { /** Waiting for some prerequisite before we can use it for the first time. */ STARTING, /** Successfully created, not waiting for any messages to be accurate. */ @@ -139,10 +139,6 @@ protected void revive() { JsRunnable.doNothing()); } - public Status getStatus() { - return status; - } - protected static FlatBufferBuilder subscriptionRequest(byte[] tableTicket, BitSet columns, @Nullable RangeSet viewport, BarrageSubscriptionOptions options, boolean isReverseViewport) { @@ -170,7 +166,7 @@ protected static FlatBufferBuilder subscriptionRequest(byte[] tableTicket, BitSe protected void sendBarrageSubscriptionRequest(RangeSet viewport, JsArray columns, Double updateIntervalMs, boolean isReverseViewport) { - if (status == Status.DONE) { + if (isClosed()) { if (failMsg == null) { throw new IllegalStateException("Can't change subscription, already closed"); } else { @@ -214,15 +210,39 @@ protected WorkerConnection connection() { return connection; } + /** + * True if the subscription is in the ACTIVE state, meaning that the server and client are in sync with the state of + * the subscription. + */ protected boolean isSubscriptionReady() { return status == Status.ACTIVE; } + /** + * Returns true if the subscription is closed and cannot be used again, false if it is actively listening for more + * data. + */ + public boolean isClosed() { + return status == Status.DONE; + } + + /** + * Returns true if the subscription is in a state where it can be used to read data, false if still waiting for the + * server to send the first snapshot or if the subscription has been closed. + * + * @return true if the {@link #size()} method will return data based on the subscription, false if some other source + * of the table's size will be used. + */ + public boolean hasValidSize() { + return status == Status.ACTIVE || status == Status.PENDING_UPDATE; + } + + public double size() { - if (status == Status.ACTIVE) { + if (hasValidSize()) { return barrageSubscription.getCurrentSize(); } - if (status == Status.DONE) { + if (isClosed()) { throw new IllegalStateException("Can't read size when already closed"); } return state.getSize(); @@ -505,7 +525,7 @@ private void onFlightData(FlightData data) { } protected void onStreamEnd(ResponseStreamWrapper.Status status) { - if (this.status == Status.DONE) { + if (isClosed()) { return; } if (status.isTransportError()) { diff --git a/web/client-api/src/main/java/io/deephaven/web/client/api/subscription/TableViewportSubscription.java b/web/client-api/src/main/java/io/deephaven/web/client/api/subscription/TableViewportSubscription.java index c199be4d497..eb19551c5e3 100644 --- a/web/client-api/src/main/java/io/deephaven/web/client/api/subscription/TableViewportSubscription.java +++ b/web/client-api/src/main/java/io/deephaven/web/client/api/subscription/TableViewportSubscription.java @@ -281,7 +281,7 @@ public void setInternalViewport(double firstRow, double lastRow, Column[] column */ @JsMethod public void close() { - if (status == Status.DONE) { + if (isClosed()) { JsLog.warn("TableViewportSubscription.close called on subscription that's already done."); } retained = false; @@ -300,14 +300,12 @@ public void internalClose() { reconnectSubscription.remove(); - if (retained || status == Status.DONE) { + if (retained || isClosed()) { // the JsTable has indicated it is no longer interested in this viewport, but other calling // code has retained it, keep it open for now. return; } - status = Status.DONE; - super.close(); }