From f6b02416abb96ad85602f683582b9cf72ad6ca67 Mon Sep 17 00:00:00 2001 From: Colin Alworth Date: Thu, 23 Jan 2025 14:53:09 -0600 Subject: [PATCH 1/3] fix: JS clients must not release tickets for pending operations 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 | 53 +++++++++++-------- 1 file changed, 32 insertions(+), 21 deletions(-) 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 ba5686df6d1..9eacd60df80 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 @@ -150,7 +150,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 @@ -348,7 +348,7 @@ private Promise makeKeyTable() { return keyTable; } - private TicketAndPromise makeView(TicketAndPromise prevTicket) { + private TicketAndPromise makeView(TicketAndPromise prevTicket) { if (viewTicket != null) { return viewTicket; } @@ -371,7 +371,30 @@ private TicketAndPromise makeView(TicketAndPromise prevTicket) { c.apply(error, null); return null; }); - }), connection); + }).then(result -> { + 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(); + then(state -> { + state.unretain(JsTreeTable.this); + return null; + }); + } + }; return viewTicket; } @@ -622,16 +645,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); @@ -644,18 +667,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, From e8414620e78f0a287fd7e7cdb28867f7e5de5139 Mon Sep 17 00:00:00 2001 From: Colin Alworth Date: Thu, 23 Jan 2025 15:57:34 -0600 Subject: [PATCH 2/3] Spotless --- .../java/io/deephaven/web/client/api/tree/JsTreeTable.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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 9eacd60df80..209bcc4fe07 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 @@ -374,8 +374,8 @@ private TicketAndPromise makeView(TicketAndPromise prevTick }).then(result -> { ClientTableState state = new ClientTableState(connection, new TableTicket(viewTicket.ticket().getTicket_asU8()), (callback, newState, metadata) -> { - callback.apply("fail, trees dont reconnect like this", null); - }, ""); + callback.apply("fail, trees dont reconnect like this", null); + }, ""); state.retain(JsTreeTable.this); ExportedTableCreationResponse def = new ExportedTableCreationResponse(); HierarchicalTableDescriptor treeDescriptor = From ee6c53fb8abdcedb54e495783f1623016be6b21c Mon Sep 17 00:00:00 2001 From: Colin Alworth Date: Mon, 27 Jan 2025 14:19:09 -0600 Subject: [PATCH 3/3] Also fix a related issue where racing view changes can log an err --- .../web/client/api/tree/JsTreeTable.java | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) 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 209bcc4fe07..1829a1ee7c6 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_core.proto.hierarchicaltable_pb.HierarchicalTableSourceExportRequest; import io.deephaven.javascript.proto.dhinternal.io.deephaven_core.proto.hierarchicaltable_pb.HierarchicalTableViewKeyTableDescriptor; import io.deephaven.javascript.proto.dhinternal.io.deephaven_core.proto.hierarchicaltable_pb.HierarchicalTableViewRequest; +import io.deephaven.javascript.proto.dhinternal.io.deephaven_core.proto.hierarchicaltable_pb_service.UnaryResponse; import io.deephaven.javascript.proto.dhinternal.io.deephaven_core.proto.table_pb.Condition; import io.deephaven.javascript.proto.dhinternal.io.deephaven_core.proto.table_pb.ExportedTableCreationResponse; import io.deephaven.javascript.proto.dhinternal.io.deephaven_core.proto.table_pb.SortDescriptor; @@ -354,24 +356,34 @@ private TicketAndPromise makeView(TicketAndPromise prevTick } 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.aborted) { + return Promise.resolve(controller.signal.reason); + } 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 -> { c.apply(error, null); return null; }); }).then(result -> { + if (controller.signal.aborted) { + return Promise.reject(controller.signal.reason); + } ClientTableState state = new ClientTableState(connection, new TableTicket(viewTicket.ticket().getTicket_asU8()), (callback, newState, metadata) -> { callback.apply("fail, trees dont reconnect like this", null); @@ -389,6 +401,7 @@ private TicketAndPromise makeView(TicketAndPromise prevTick @Override public void release() { super.release(); + controller.abort(); then(state -> { state.unretain(JsTreeTable.this); return null;