Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: JS subscriptions should return correct size while updating (#6463) #6464

Merged
merged 2 commits into from
Dec 10, 2024
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -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.
Original file line number Diff line number Diff line change
@@ -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<Column> 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()) {
Original file line number Diff line number Diff line change
@@ -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();
}