From 7600459da160ddf02b4460d24f665dc3ef93ce69 Mon Sep 17 00:00:00 2001 From: Colin Alworth Date: Wed, 29 Jan 2025 09:23:17 -0600 Subject: [PATCH 1/2] 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 --- .../web/client/api/tree/JsTreeTable.java | 71 +++++++++++++------ .../web/client/fu/JsAbortSignal.java | 15 ++++ 2 files changed, 63 insertions(+), 23 deletions(-) create mode 100644 web/client-api/src/main/java/io/deephaven/web/client/fu/JsAbortSignal.java diff --git a/web/client-api/src/main/java/io/deephaven/web/client/api/tree/JsTreeTable.java b/web/client-api/src/main/java/io/deephaven/web/client/api/tree/JsTreeTable.java index 286a6c4213a..6f19a58aae6 100644 --- a/web/client-api/src/main/java/io/deephaven/web/client/api/tree/JsTreeTable.java +++ b/web/client-api/src/main/java/io/deephaven/web/client/api/tree/JsTreeTable.java @@ -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; @@ -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; @@ -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; @@ -149,7 +152,7 @@ private enum RebuildStep { private Object[][] keyTableData; private Promise keyTable; - private TicketAndPromise viewTicket; + private TicketAndPromise viewTicket; private Promise stream; // 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 makeKeyTable() { return keyTable; } - private TicketAndPromise makeView(TicketAndPromise prevTicket) { + private TicketAndPromise makeView(TicketAndPromise prevTicket) { if (viewTicket != null) { return viewTicket; } Ticket ticket = connection.getConfig().newTicket(); Promise 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.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.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; } @@ -621,16 +658,16 @@ private void replaceSubscription(RebuildStep step) { Promise 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 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); @@ -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, diff --git a/web/client-api/src/main/java/io/deephaven/web/client/fu/JsAbortSignal.java b/web/client-api/src/main/java/io/deephaven/web/client/fu/JsAbortSignal.java new file mode 100644 index 00000000000..2f21d16633d --- /dev/null +++ b/web/client-api/src/main/java/io/deephaven/web/client/fu/JsAbortSignal.java @@ -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(); +} From 96f6d7f2563bae84998aefaedf0bffd40b579159 Mon Sep 17 00:00:00 2001 From: Colin Alworth Date: Mon, 3 Feb 2025 15:30:07 -0600 Subject: [PATCH 2/2] Spotless --- .../main/java/io/deephaven/web/client/fu/JsAbortSignal.java | 3 +++ 1 file changed, 3 insertions(+) diff --git a/web/client-api/src/main/java/io/deephaven/web/client/fu/JsAbortSignal.java b/web/client-api/src/main/java/io/deephaven/web/client/fu/JsAbortSignal.java index 2f21d16633d..6eaf5041f95 100644 --- a/web/client-api/src/main/java/io/deephaven/web/client/fu/JsAbortSignal.java +++ b/web/client-api/src/main/java/io/deephaven/web/client/fu/JsAbortSignal.java @@ -1,3 +1,6 @@ +// +// Copyright (c) 2016-2024 Deephaven Data Labs and Patent Pending +// package io.deephaven.web.client.fu; import elemental2.dom.AbortSignal;