Skip to content

Commit 7600459

Browse files
committed
fix: JS clients must not release tickets for pending operations (#6591)
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
1 parent 1affe97 commit 7600459

File tree

2 files changed

+63
-23
lines changed

2 files changed

+63
-23
lines changed

web/client-api/src/main/java/io/deephaven/web/client/api/tree/JsTreeTable.java

+48-23
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010
import elemental2.core.JsArray;
1111
import elemental2.core.JsObject;
1212
import elemental2.core.Uint8Array;
13+
import elemental2.dom.AbortController;
1314
import elemental2.dom.DomGlobal;
1415
import elemental2.promise.IThenable;
1516
import elemental2.promise.Promise;
@@ -18,6 +19,7 @@
1819
import io.deephaven.javascript.proto.dhinternal.io.deephaven.proto.hierarchicaltable_pb.HierarchicalTableSourceExportRequest;
1920
import io.deephaven.javascript.proto.dhinternal.io.deephaven.proto.hierarchicaltable_pb.HierarchicalTableViewKeyTableDescriptor;
2021
import io.deephaven.javascript.proto.dhinternal.io.deephaven.proto.hierarchicaltable_pb.HierarchicalTableViewRequest;
22+
import io.deephaven.javascript.proto.dhinternal.io.deephaven.proto.hierarchicaltable_pb_service.UnaryResponse;
2123
import io.deephaven.javascript.proto.dhinternal.io.deephaven.proto.table_pb.Condition;
2224
import io.deephaven.javascript.proto.dhinternal.io.deephaven.proto.table_pb.ExportedTableCreationResponse;
2325
import io.deephaven.javascript.proto.dhinternal.io.deephaven.proto.table_pb.TableReference;
@@ -36,6 +38,7 @@
3638
import io.deephaven.web.client.api.subscription.AbstractTableSubscription;
3739
import io.deephaven.web.client.api.subscription.SubscriptionType;
3840
import io.deephaven.web.client.api.widget.JsWidget;
41+
import io.deephaven.web.client.fu.JsAbortSignal;
3942
import io.deephaven.web.client.fu.JsItr;
4043
import io.deephaven.web.client.fu.JsLog;
4144
import io.deephaven.web.client.fu.LazyPromise;
@@ -149,7 +152,7 @@ private enum RebuildStep {
149152
private Object[][] keyTableData;
150153
private Promise<JsTable> keyTable;
151154

152-
private TicketAndPromise<?> viewTicket;
155+
private TicketAndPromise<ClientTableState> viewTicket;
153156
private Promise<TreeSubscription> stream;
154157

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

350-
private TicketAndPromise<?> makeView(TicketAndPromise<?> prevTicket) {
353+
private TicketAndPromise<ClientTableState> makeView(TicketAndPromise<?> prevTicket) {
351354
if (viewTicket != null) {
352355
return viewTicket;
353356
}
354357
Ticket ticket = connection.getConfig().newTicket();
355358
Promise<JsTable> keyTable = makeKeyTable();
359+
AbortController controller = new AbortController();
360+
356361
viewTicket = new TicketAndPromise<>(ticket, Callbacks.grpcUnaryPromise(c -> {
357362
HierarchicalTableViewRequest viewRequest = new HierarchicalTableViewRequest();
358363
viewRequest.setHierarchicalTableId(prevTicket.ticket());
359364
viewRequest.setResultViewId(ticket);
360365
keyTable.then(t -> {
366+
if (controller.signal.isAborted()) {
367+
return Promise.reject(Js.<JsAbortSignal>cast(controller.signal).getReason());
368+
}
361369
if (keyTableData[0].length > 0) {
362370
HierarchicalTableViewKeyTableDescriptor expansions = new HierarchicalTableViewKeyTableDescriptor();
363371
expansions.setKeyTableId(t.getHandle().makeTicket());
364372
expansions.setKeyTableActionColumn(actionCol.getName());
365373
viewRequest.setExpansions(expansions);
366374
}
367-
connection.hierarchicalTableServiceClient().view(viewRequest, connection.metadata(), c::apply);
375+
UnaryResponse viewCreationCall =
376+
connection.hierarchicalTableServiceClient().view(viewRequest, connection.metadata(), c::apply);
377+
controller.signal.addEventListener("abort", e -> viewCreationCall.cancel());
368378
return null;
369-
}, error -> {
379+
}).catch_(error -> {
370380
c.apply(error, null);
371381
return null;
372382
});
373-
}), connection);
383+
}).then(result -> {
384+
if (controller.signal.isAborted()) {
385+
return Promise.reject(Js.<JsAbortSignal>cast(controller.signal).getReason());
386+
}
387+
ClientTableState state = new ClientTableState(connection,
388+
new TableTicket(viewTicket.ticket().getTicket_asU8()), (callback, newState, metadata) -> {
389+
callback.apply("fail, trees dont reconnect like this", null);
390+
}, "");
391+
state.retain(JsTreeTable.this);
392+
ExportedTableCreationResponse def = new ExportedTableCreationResponse();
393+
HierarchicalTableDescriptor treeDescriptor =
394+
HierarchicalTableDescriptor.deserializeBinary(widget.getDataAsU8());
395+
def.setSchemaHeader(treeDescriptor.getSnapshotSchema_asU8());
396+
def.setResultId(new TableReference());
397+
def.getResultId().setTicket(viewTicket.ticket());
398+
state.applyTableCreationResponse(def);
399+
return Promise.resolve(state);
400+
}), connection) {
401+
@Override
402+
public void release() {
403+
super.release();
404+
controller.abort();
405+
then(state -> {
406+
state.unretain(JsTreeTable.this);
407+
return null;
408+
});
409+
}
410+
};
374411
return viewTicket;
375412
}
376413

@@ -621,16 +658,16 @@ private void replaceSubscription(RebuildStep step) {
621658
Promise<TreeSubscription> stream = Promise.resolve(defer())
622659
.then(ignore -> {
623660
makeKeyTable();
624-
TicketAndPromise filter = prepareFilter();
625-
TicketAndPromise sort = prepareSort(filter);
626-
TicketAndPromise view = makeView(sort);
661+
TicketAndPromise<?> filter = prepareFilter();
662+
TicketAndPromise<?> sort = prepareSort(filter);
663+
TicketAndPromise<ClientTableState> view = makeView(sort);
627664
return Promise.all(
628665
keyTable,
629666
filter.promise(),
630-
sort.promise(),
631-
view.promise());
667+
sort.promise())
668+
.then(others -> view.promise());
632669
})
633-
.then(results -> {
670+
.then(state -> {
634671
BitSet columnsBitset = makeColumnSubscriptionBitset();
635672
RangeSet range = RangeSet.ofRange((long) (double) firstRow, (long) (double) lastRow);
636673

@@ -643,18 +680,6 @@ private void replaceSubscription(RebuildStep step) {
643680
range,
644681
alwaysFireEvent);
645682

646-
ClientTableState state = new ClientTableState(connection,
647-
new TableTicket(viewTicket.ticket().getTicket_asU8()), (callback, newState, metadata) -> {
648-
callback.apply("fail, trees dont reconnect like this", null);
649-
}, "");
650-
ExportedTableCreationResponse def = new ExportedTableCreationResponse();
651-
HierarchicalTableDescriptor treeDescriptor =
652-
HierarchicalTableDescriptor.deserializeBinary(widget.getDataAsU8());
653-
def.setSchemaHeader(treeDescriptor.getSnapshotSchema_asU8());
654-
def.setResultId(new TableReference());
655-
def.getResultId().setTicket(viewTicket.ticket());
656-
state.applyTableCreationResponse(def);
657-
658683
TreeSubscription subscription = new TreeSubscription(state, connection);
659684

660685
subscription.addEventListener(TreeSubscription.EVENT_UPDATED,
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
package io.deephaven.web.client.fu;
2+
3+
import elemental2.dom.AbortSignal;
4+
import jsinterop.annotations.JsPackage;
5+
import jsinterop.annotations.JsProperty;
6+
import jsinterop.annotations.JsType;
7+
8+
/**
9+
* Backport of elemental2 1.2.3's change to include the reason field.
10+
*/
11+
@JsType(isNative = true, namespace = JsPackage.GLOBAL, name = "AbortSignal")
12+
public interface JsAbortSignal extends AbortSignal {
13+
@JsProperty
14+
Object getReason();
15+
}

0 commit comments

Comments
 (0)