Skip to content

Commit

Permalink
fix: JS clients must not release tickets for pending operations (#6591)
Browse files Browse the repository at this point in the history 
When a necessary operation is still running, clients must not release
upstream tickets. This has mostly worked by the server running fast
enough or by the websocket transport serializing operations in a way
that we can't always rely on.

Testing this is difficult to reliably do - the ticket has some manual
steps that generally make it possible to see the issue, and we're
exploring other options to make bugs like this more obvious. At this
time, there is no good way to put an integration test in that verifies
correct behavior efficiently.

Fixes #6056
Fixes DH-18486
  • Loading branch information
@niloc132
niloc132 committed Feb 3, 2025
1 parent 1affe97 commit 7600459
Show file tree
Hide file tree
Showing 2 changed files with 63 additions and 23 deletions.
71 changes: 48 additions & 23 deletions web/client-api/src/main/java/io/deephaven/web/client/api/tree/JsTreeTable.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
import elemental2.core.JsArray;
import elemental2.core.JsObject;
import elemental2.core.Uint8Array;
import elemental2.dom.AbortController;
import elemental2.dom.DomGlobal;
import elemental2.promise.IThenable;
import elemental2.promise.Promise;
Expand All @@ -18,6 +19,7 @@
import io.deephaven.javascript.proto.dhinternal.io.deephaven.proto.hierarchicaltable_pb.HierarchicalTableSourceExportRequest;
import io.deephaven.javascript.proto.dhinternal.io.deephaven.proto.hierarchicaltable_pb.HierarchicalTableViewKeyTableDescriptor;
import io.deephaven.javascript.proto.dhinternal.io.deephaven.proto.hierarchicaltable_pb.HierarchicalTableViewRequest;
import io.deephaven.javascript.proto.dhinternal.io.deephaven.proto.hierarchicaltable_pb_service.UnaryResponse;
import io.deephaven.javascript.proto.dhinternal.io.deephaven.proto.table_pb.Condition;
import io.deephaven.javascript.proto.dhinternal.io.deephaven.proto.table_pb.ExportedTableCreationResponse;
import io.deephaven.javascript.proto.dhinternal.io.deephaven.proto.table_pb.TableReference;
Expand All @@ -36,6 +38,7 @@
import io.deephaven.web.client.api.subscription.AbstractTableSubscription;
import io.deephaven.web.client.api.subscription.SubscriptionType;
import io.deephaven.web.client.api.widget.JsWidget;
import io.deephaven.web.client.fu.JsAbortSignal;
import io.deephaven.web.client.fu.JsItr;
import io.deephaven.web.client.fu.JsLog;
import io.deephaven.web.client.fu.LazyPromise;
Expand Down Expand Up @@ -149,7 +152,7 @@ private enum RebuildStep {
private Object[][] keyTableData;
private Promise<JsTable> keyTable;

private TicketAndPromise<?> viewTicket;
private TicketAndPromise<ClientTableState> viewTicket;
private Promise<TreeSubscription> stream;

// the "next" set of filters/sorts that we'll use. these either are "==" to the above fields, or are scheduled
Expand Down Expand Up @@ -347,30 +350,64 @@ private Promise<JsTable> makeKeyTable() {
return keyTable;
}

private TicketAndPromise<?> makeView(TicketAndPromise<?> prevTicket) {
private TicketAndPromise<ClientTableState> makeView(TicketAndPromise<?> prevTicket) {
if (viewTicket != null) {
return viewTicket;
}
Ticket ticket = connection.getConfig().newTicket();
Promise<JsTable> keyTable = makeKeyTable();
AbortController controller = new AbortController();

viewTicket = new TicketAndPromise<>(ticket, Callbacks.grpcUnaryPromise(c -> {
HierarchicalTableViewRequest viewRequest = new HierarchicalTableViewRequest();
viewRequest.setHierarchicalTableId(prevTicket.ticket());
viewRequest.setResultViewId(ticket);
keyTable.then(t -> {
if (controller.signal.isAborted()) {
return Promise.reject(Js.<JsAbortSignal>cast(controller.signal).getReason());
}
if (keyTableData[0].length > 0) {
HierarchicalTableViewKeyTableDescriptor expansions = new HierarchicalTableViewKeyTableDescriptor();
expansions.setKeyTableId(t.getHandle().makeTicket());
expansions.setKeyTableActionColumn(actionCol.getName());
viewRequest.setExpansions(expansions);
}
connection.hierarchicalTableServiceClient().view(viewRequest, connection.metadata(), c::apply);
UnaryResponse viewCreationCall =
connection.hierarchicalTableServiceClient().view(viewRequest, connection.metadata(), c::apply);
controller.signal.addEventListener("abort", e -> viewCreationCall.cancel());
return null;
}, error -> {
}).catch_(error -> {
c.apply(error, null);
return null;
});
}), connection);
}).then(result -> {
if (controller.signal.isAborted()) {
return Promise.reject(Js.<JsAbortSignal>cast(controller.signal).getReason());
}
ClientTableState state = new ClientTableState(connection,
new TableTicket(viewTicket.ticket().getTicket_asU8()), (callback, newState, metadata) -> {
callback.apply("fail, trees dont reconnect like this", null);
}, "");
state.retain(JsTreeTable.this);
ExportedTableCreationResponse def = new ExportedTableCreationResponse();
HierarchicalTableDescriptor treeDescriptor =
HierarchicalTableDescriptor.deserializeBinary(widget.getDataAsU8());
def.setSchemaHeader(treeDescriptor.getSnapshotSchema_asU8());
def.setResultId(new TableReference());
def.getResultId().setTicket(viewTicket.ticket());
state.applyTableCreationResponse(def);
return Promise.resolve(state);
}), connection) {
@Override
public void release() {
super.release();
controller.abort();
then(state -> {
state.unretain(JsTreeTable.this);
return null;
});
}
};
return viewTicket;
}

Expand Down Expand Up @@ -621,16 +658,16 @@ private void replaceSubscription(RebuildStep step) {
Promise<TreeSubscription> stream = Promise.resolve(defer())
.then(ignore -> {
makeKeyTable();
TicketAndPromise filter = prepareFilter();
TicketAndPromise sort = prepareSort(filter);
TicketAndPromise view = makeView(sort);
TicketAndPromise<?> filter = prepareFilter();
TicketAndPromise<?> sort = prepareSort(filter);
TicketAndPromise<ClientTableState> view = makeView(sort);
return Promise.all(
keyTable,
filter.promise(),
sort.promise(),
view.promise());
sort.promise())
.then(others -> view.promise());
})
.then(results -> {
.then(state -> {
BitSet columnsBitset = makeColumnSubscriptionBitset();
RangeSet range = RangeSet.ofRange((long) (double) firstRow, (long) (double) lastRow);

Expand All @@ -643,18 +680,6 @@ private void replaceSubscription(RebuildStep step) {
range,
alwaysFireEvent);

ClientTableState state = new ClientTableState(connection,
new TableTicket(viewTicket.ticket().getTicket_asU8()), (callback, newState, metadata) -> {
callback.apply("fail, trees dont reconnect like this", null);
}, "");
ExportedTableCreationResponse def = new ExportedTableCreationResponse();
HierarchicalTableDescriptor treeDescriptor =
HierarchicalTableDescriptor.deserializeBinary(widget.getDataAsU8());
def.setSchemaHeader(treeDescriptor.getSnapshotSchema_asU8());
def.setResultId(new TableReference());
def.getResultId().setTicket(viewTicket.ticket());
state.applyTableCreationResponse(def);

TreeSubscription subscription = new TreeSubscription(state, connection);

subscription.addEventListener(TreeSubscription.EVENT_UPDATED,
Expand Down
15 changes: 15 additions & 0 deletions web/client-api/src/main/java/io/deephaven/web/client/fu/JsAbortSignal.java
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
package io.deephaven.web.client.fu;

import elemental2.dom.AbortSignal;
import jsinterop.annotations.JsPackage;
import jsinterop.annotations.JsProperty;
import jsinterop.annotations.JsType;

/**
* Backport of elemental2 1.2.3's change to include the reason field.
*/
@JsType(isNative = true, namespace = JsPackage.GLOBAL, name = "AbortSignal")
public interface JsAbortSignal extends AbortSignal {
@JsProperty
Object getReason();
}

0 comments on commit 7600459

Please sign in to comment.