From f48b18f8515afca4d3f17880d41c9e2c00bea246 Mon Sep 17 00:00:00 2001 From: mikebender Date: Thu, 30 Nov 2023 12:03:08 -0500 Subject: [PATCH 01/20] fix: Handle exported objects fetches more correctly - Previously, exported objects were not handling their fetched objects correctly - Tables were not using copies, only widgets were - The parent ticket was not being released in many cases (TreeTable, PartitonedTable, PandasTable) - Refactored so exported objects always follow the same path - Just call connection.getObject, which always creates a new ticket for the object - Still need to close the widget so it closes the exported objects - Test by adding debug logging for SessionState: ```py import jpy logger = jpy.get_type('org.slf4j.LoggerFactory').getLogger('io.deephaven.server.session.SessionState') jpy.cast(logger, jpy.get_type('ch.qos.logback.classic.Logger')).setLevel(jpy.get_type('ch.qos.logback.classic.Level').DEBUG) ``` --- .../web/client/api/JsPartitionedTable.java | 11 +++++++- .../web/client/api/WorkerConnection.java | 15 +++++++++- .../web/client/api/tree/JsTreeTable.java | 3 +- .../api/widget/JsWidgetExportedObject.java | 28 +++++-------------- 4 files changed, 32 insertions(+), 25 deletions(-) diff --git a/web/client-api/src/main/java/io/deephaven/web/client/api/JsPartitionedTable.java b/web/client-api/src/main/java/io/deephaven/web/client/api/JsPartitionedTable.java index 897e616ebc5..67468386477 100644 --- a/web/client-api/src/main/java/io/deephaven/web/client/api/JsPartitionedTable.java +++ b/web/client-api/src/main/java/io/deephaven/web/client/api/JsPartitionedTable.java @@ -108,7 +108,14 @@ public Promise refetch() { this.columns = JsObject.freeze(columns); this.keyColumns = JsObject.freeze(keyColumns); - return w.getExportedObjects()[0].fetch(); + + Promise promise = w.getExportedObjects()[0].fetch(); + promise.then(ignore -> { + // We only need to keep the widget open long enough to get the exported objects + w.close(); + return null; + }); + return promise; }).then(result -> { keys = (JsTable) result; // TODO(deephaven-core#3604) in case of a new session, we should do a full refetch @@ -308,6 +315,8 @@ public Promise getKeyTable() { * will not affect tables in use. */ public void close() { + widget.close(); + if (keys != null) { keys.close(); } diff --git a/web/client-api/src/main/java/io/deephaven/web/client/api/WorkerConnection.java b/web/client-api/src/main/java/io/deephaven/web/client/api/WorkerConnection.java index f9611a1bc57..90ef1a9352d 100644 --- a/web/client-api/src/main/java/io/deephaven/web/client/api/WorkerConnection.java +++ b/web/client-api/src/main/java/io/deephaven/web/client/api/WorkerConnection.java @@ -759,6 +759,11 @@ public Promise getTable(JsVariableDefinition varDef, @Nullable Boolean }); } + public Promise getObject(TypedTicket typedTicket) { + return getObject( + new JsVariableDefinition(typedTicket.getType(), null, typedTicket.getTicket().getTicket_asB64(), null)); + } + public Promise getObject(JsVariableDefinition definition) { if (JsVariableType.TABLE.equalsIgnoreCase(definition.getType())) { return getTable(definition, null); @@ -766,7 +771,15 @@ public Promise getObject(JsVariableDefinition definition) { return getFigure(definition); } else if (JsVariableType.PANDAS.equalsIgnoreCase(definition.getType())) { return getWidget(definition) - .then(widget -> widget.getExportedObjects()[0].fetch()); + .then(widget -> { + Promise promise = widget.getExportedObjects()[0].fetch(); + promise.then(ignore -> { + // We only need to keep the widget open long enough to get the exported objects + widget.close(); + return null; + }); + return promise; + }); } else if (JsVariableType.PARTITIONEDTABLE.equalsIgnoreCase(definition.getType())) { return getPartitionedTable(definition); } else if (JsVariableType.HIERARCHICALTABLE.equalsIgnoreCase(definition.getType())) { 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 a1f0569955c..c6ffa93cbd3 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 @@ -979,8 +979,7 @@ public void close() { connection.unregisterSimpleReconnectable(this); - // Presently it is never necessary to release widget tickets, since they can't be export tickets. - // connection.releaseTicket(widget.getTicket()); + connection.releaseTicket(widget.getTicket()); if (filteredTable != null) { filteredTable.release(); diff --git a/web/client-api/src/main/java/io/deephaven/web/client/api/widget/JsWidgetExportedObject.java b/web/client-api/src/main/java/io/deephaven/web/client/api/widget/JsWidgetExportedObject.java index a8948d990fa..31ac132481e 100644 --- a/web/client-api/src/main/java/io/deephaven/web/client/api/widget/JsWidgetExportedObject.java +++ b/web/client-api/src/main/java/io/deephaven/web/client/api/widget/JsWidgetExportedObject.java @@ -6,15 +6,10 @@ import com.vertispan.tsdefs.annotations.TsInterface; import com.vertispan.tsdefs.annotations.TsName; import elemental2.promise.Promise; -import io.deephaven.javascript.proto.dhinternal.io.deephaven.proto.table_pb.ExportedTableCreationResponse; import io.deephaven.javascript.proto.dhinternal.io.deephaven.proto.ticket_pb.TypedTicket; -import io.deephaven.web.client.api.Callbacks; -import io.deephaven.web.client.api.JsTable; import io.deephaven.web.client.api.ServerObject; import io.deephaven.web.client.api.WorkerConnection; import io.deephaven.web.client.api.console.JsVariableDefinition; -import io.deephaven.web.client.api.console.JsVariableType; -import io.deephaven.web.client.state.ClientTableState; import jsinterop.annotations.JsMethod; import jsinterop.annotations.JsProperty; @@ -47,24 +42,15 @@ public TypedTicket typedTicket() { return typedTicket; } + /** + * Fetches the object from the server. Note that the returned object has it's own ticket and must be closed, and you + * must call {@link #close()} on this object as well whether you fetch the exported object or not. + * + * @return a promise that resolves to the object + */ @JsMethod public Promise fetch() { - if (getType().equals(JsVariableType.TABLE)) { - return Callbacks.grpcUnaryPromise(c -> { - connection.tableServiceClient().getExportedTableCreationResponse(ticket.getTicket(), - connection.metadata(), - c::apply); - }).then(etcr -> { - ClientTableState cts = connection.newStateFromUnsolicitedTable(etcr, "table for widget"); - JsTable table = new JsTable(connection, cts); - // never attempt a reconnect, since we might have a different widget schema entirely - table.addEventListener(JsTable.EVENT_DISCONNECT, ignore -> table.close()); - return Promise.resolve(table); - }); - } else { - return this.connection.getObject( - new JsVariableDefinition(ticket.getType(), null, ticket.getTicket().getTicket_asB64(), null)); - } + return this.connection.getObject(ticket); } /** From 58aeab4330f3be2824ef0ddf37498885c6357622 Mon Sep 17 00:00:00 2001 From: mikebender Date: Fri, 1 Dec 2023 11:41:58 -0500 Subject: [PATCH 02/20] Release the widget after getting the figure --- .../main/java/io/deephaven/web/client/api/WorkerConnection.java | 1 + 1 file changed, 1 insertion(+) diff --git a/web/client-api/src/main/java/io/deephaven/web/client/api/WorkerConnection.java b/web/client-api/src/main/java/io/deephaven/web/client/api/WorkerConnection.java index 90ef1a9352d..5852910f997 100644 --- a/web/client-api/src/main/java/io/deephaven/web/client/api/WorkerConnection.java +++ b/web/client-api/src/main/java/io/deephaven/web/client/api/WorkerConnection.java @@ -946,6 +946,7 @@ public Promise getFigure(JsVariableDefinition varDef) { legacyResponse.setTypedExportIdsList(Arrays.stream(widget.getExportedObjects()) .map(JsWidgetExportedObject::typedTicket).toArray(TypedTicket[]::new)); c.apply(null, legacyResponse); + widget.close(); return null; }, error -> { c.apply(error, null); From 6f77c64811862eac38c5fdeec5791811d8757d80 Mon Sep 17 00:00:00 2001 From: mikebender Date: Fri, 1 Dec 2023 11:57:23 -0500 Subject: [PATCH 03/20] Fix up pandas table fetch - Now it doesn't need to do the extra round trip --- .../web/client/api/WorkerConnection.java | 30 ++++++++++++------- 1 file changed, 20 insertions(+), 10 deletions(-) diff --git a/web/client-api/src/main/java/io/deephaven/web/client/api/WorkerConnection.java b/web/client-api/src/main/java/io/deephaven/web/client/api/WorkerConnection.java index 5852910f997..53b9080e016 100644 --- a/web/client-api/src/main/java/io/deephaven/web/client/api/WorkerConnection.java +++ b/web/client-api/src/main/java/io/deephaven/web/client/api/WorkerConnection.java @@ -770,16 +770,7 @@ public Promise getObject(JsVariableDefinition definition) { } else if (JsVariableType.FIGURE.equalsIgnoreCase(definition.getType())) { return getFigure(definition); } else if (JsVariableType.PANDAS.equalsIgnoreCase(definition.getType())) { - return getWidget(definition) - .then(widget -> { - Promise promise = widget.getExportedObjects()[0].fetch(); - promise.then(ignore -> { - // We only need to keep the widget open long enough to get the exported objects - widget.close(); - return null; - }); - return promise; - }); + return getPandasTable(definition); } else if (JsVariableType.PARTITIONEDTABLE.equalsIgnoreCase(definition.getType())) { return getPartitionedTable(definition); } else if (JsVariableType.HIERARCHICALTABLE.equalsIgnoreCase(definition.getType())) { @@ -955,6 +946,25 @@ public Promise getFigure(JsVariableDefinition varDef) { }).refetch()); } + public Promise getPandasTable(JsVariableDefinition definition) { + return getWidget(definition) + .then(widget -> { + return Callbacks.grpcUnaryPromise(c -> { + tableServiceClient().getExportedTableCreationResponse(widget.getExportedObjects()[0].typedTicket().getTicket(), metadata(), c::apply); + }).then(etcr -> { + ClientTableState cts = newStateFromUnsolicitedTable(etcr, "table for pandas"); + JsTable table = new JsTable(this, cts); + // TODO(deephaven-core#3604) if using a new session don't attempt a reconnect + // never attempt a reconnect, since we might have a different widget schema entirely + table.addEventListener(JsTable.EVENT_DISCONNECT, ignore -> table.close()); + return Promise.resolve(table); + }).then(table -> { + widget.close(); + return Promise.resolve(table); + }); + }); + } + private TypedTicket createTypedTicket(JsVariableDefinition varDef) { TypedTicket typedTicket = new TypedTicket(); typedTicket.setTicket(TableTicket.createTicket(varDef)); From 75ce4fb479d8848bce5f2dbf25a981e32c47a9c1 Mon Sep 17 00:00:00 2001 From: mikebender Date: Fri, 1 Dec 2023 17:36:54 -0500 Subject: [PATCH 04/20] WIP need to finish and clean it up - Have the exported object "own" the ticket until it is taken from it --- .../web/client/api/WorkerConnection.java | 2 +- .../web/client/api/widget/JsWidget.java | 13 +++++++++ .../api/widget/JsWidgetExportedObject.java | 29 +++++++++++++++++-- 3 files changed, 40 insertions(+), 4 deletions(-) diff --git a/web/client-api/src/main/java/io/deephaven/web/client/api/WorkerConnection.java b/web/client-api/src/main/java/io/deephaven/web/client/api/WorkerConnection.java index 53b9080e016..0be6c7e5abe 100644 --- a/web/client-api/src/main/java/io/deephaven/web/client/api/WorkerConnection.java +++ b/web/client-api/src/main/java/io/deephaven/web/client/api/WorkerConnection.java @@ -759,7 +759,7 @@ public Promise getTable(JsVariableDefinition varDef, @Nullable Boolean }); } - public Promise getObject(TypedTicket typedTicket) { + public Promise getObject(TypedTicket typedTicket, boolean exportNew) { return getObject( new JsVariableDefinition(typedTicket.getType(), null, typedTicket.getTicket().getTicket_asB64(), null)); } diff --git a/web/client-api/src/main/java/io/deephaven/web/client/api/widget/JsWidget.java b/web/client-api/src/main/java/io/deephaven/web/client/api/widget/JsWidget.java index e5449ec42a9..986a0111ef1 100644 --- a/web/client-api/src/main/java/io/deephaven/web/client/api/widget/JsWidget.java +++ b/web/client-api/src/main/java/io/deephaven/web/client/api/widget/JsWidget.java @@ -85,6 +85,18 @@ private void closeStream() { hasFetched = false; } + /** + * Also closes all the exported objects that the widget still owns + */ + private void closeExportedObjects() { + for (int i = 0; i < exportedObjects.length; i++) { + JsWidgetExportedObject exportedObject = exportedObjects.getAt(i); + if (exportedObject.isTicketOwner()) { + exportedObject.close(); + } + } + } + /** * Ends the client connection to the server. */ @@ -92,6 +104,7 @@ private void closeStream() { public void close() { suppressEvents(); closeStream(); + closeExportedObjects(); connection.releaseTicket(getTicket()); } diff --git a/web/client-api/src/main/java/io/deephaven/web/client/api/widget/JsWidgetExportedObject.java b/web/client-api/src/main/java/io/deephaven/web/client/api/widget/JsWidgetExportedObject.java index 31ac132481e..31ef2944c63 100644 --- a/web/client-api/src/main/java/io/deephaven/web/client/api/widget/JsWidgetExportedObject.java +++ b/web/client-api/src/main/java/io/deephaven/web/client/api/widget/JsWidgetExportedObject.java @@ -11,6 +11,7 @@ import io.deephaven.web.client.api.WorkerConnection; import io.deephaven.web.client.api.console.JsVariableDefinition; import jsinterop.annotations.JsMethod; +import jsinterop.annotations.JsOptional; import jsinterop.annotations.JsProperty; /** @@ -24,6 +25,9 @@ public class JsWidgetExportedObject implements ServerObject { private final TypedTicket ticket; + /** Whether this exported object instance is the owner of the ticket or not. */ + private final boolean isTicketOwner = true; + public JsWidgetExportedObject(WorkerConnection connection, TypedTicket ticket) { this.connection = connection; this.ticket = ticket; @@ -42,14 +46,30 @@ public TypedTicket typedTicket() { return typedTicket; } + public boolean isTicketOwner() { + return isTicketOwner; + } + + public Promise fetch() { + return fetch(false); + } + /** - * Fetches the object from the server. Note that the returned object has it's own ticket and must be closed, and you - * must call {@link #close()} on this object as well whether you fetch the exported object or not. + * Fetches the object from the server. + * @param takeOwnership Whether to take ownership of the object. Defaults to false. * * @return a promise that resolves to the object */ @JsMethod - public Promise fetch() { + public Promise fetch(@JsOptional Boolean takeOwnership) { + if (takeOwnership == null) { + takeOwnership = false; + } + if (takeOwnership) { + // TODO: if takeOwnership, don't want to create a new exportScopeTicket... + isTicketOwner = false; + throw new RuntimeException("Haven't implemented this yet"); + } return this.connection.getObject(ticket); } @@ -59,6 +79,9 @@ public Promise fetch() { */ @JsMethod public void close() { + if (!isTicketOwner) { + throw new IllegalStateException("Exported object is no longer the owner of this ticket."); + } connection.releaseTicket(ticket.getTicket()); } } From f8d8737932c6feef82b256fc8324528eb00efa79 Mon Sep 17 00:00:00 2001 From: Mike Bender Date: Sat, 2 Dec 2023 00:17:37 -0500 Subject: [PATCH 05/20] fix: Clean up how objects are managed when they are fetched - Clean up memory leak in JsPartitionedTable, JsFigure, PandasTable with widgets not being closed after being used - Add a `takeTicket` method to JsWidgetExportedObject to make it clear when ownership of the ticket has been passed on - Close exported objects that are still owned when closing a widget - Tested using the steps and scripts provided in the ticket, as well as testing with creating some tables and opening/closing them in the UI - Fixes #4903 --- .../web/client/api/JsPartitionedTable.java | 2 +- .../web/client/api/WorkerConnection.java | 207 ++++++++++++++---- .../web/client/api/widget/JsWidget.java | 3 +- .../api/widget/JsWidgetExportedObject.java | 33 ++- .../deephaven/web/client/ide/IdeSession.java | 4 +- 5 files changed, 190 insertions(+), 59 deletions(-) diff --git a/web/client-api/src/main/java/io/deephaven/web/client/api/JsPartitionedTable.java b/web/client-api/src/main/java/io/deephaven/web/client/api/JsPartitionedTable.java index 67468386477..44ccb4e0a31 100644 --- a/web/client-api/src/main/java/io/deephaven/web/client/api/JsPartitionedTable.java +++ b/web/client-api/src/main/java/io/deephaven/web/client/api/JsPartitionedTable.java @@ -109,7 +109,7 @@ public Promise refetch() { this.keyColumns = JsObject.freeze(keyColumns); - Promise promise = w.getExportedObjects()[0].fetch(); + Promise promise = w.getExportedObjects()[0].fetch(true); promise.then(ignore -> { // We only need to keep the widget open long enough to get the exported objects w.close(); diff --git a/web/client-api/src/main/java/io/deephaven/web/client/api/WorkerConnection.java b/web/client-api/src/main/java/io/deephaven/web/client/api/WorkerConnection.java index 0be6c7e5abe..0d7d23b493d 100644 --- a/web/client-api/src/main/java/io/deephaven/web/client/api/WorkerConnection.java +++ b/web/client-api/src/main/java/io/deephaven/web/client/api/WorkerConnection.java @@ -759,11 +759,63 @@ public Promise getTable(JsVariableDefinition varDef, @Nullable Boolean }); } - public Promise getObject(TypedTicket typedTicket, boolean exportNew) { - return getObject( - new JsVariableDefinition(typedTicket.getType(), null, typedTicket.getTicket().getTicket_asB64(), null)); + /** + * Export a table from the provided ticket + * + * @param ticket Ticket of the table to export + * @param fetchSummary Description for the fetch. Used for logging and errors. + * @return Promise for the table + */ + public Promise getTable(TypedTicket ticket, String fetchSummary) { + // Note that creating a CTS like this means we can't actually refetch it, but that's okay, we can't + // reconnect in this way without refetching the entire figure anyway. + return Callbacks.grpcUnaryPromise(c -> { + tableServiceClient().getExportedTableCreationResponse(ticket.getTicket(), + metadata(), + c::apply); + }).then(etcr -> { + ClientTableState cts = newStateFromUnsolicitedTable(etcr, fetchSummary); + JsTable table = new JsTable(this, cts); + return Promise.resolve(table); + }); + } + + /** + * Get an object from it's ticket + * + * @param typedTicket Ticket to get the object for + * @return The object requested + */ + public Promise getObject(TypedTicket typedTicket) { + if (JsVariableType.TABLE.equalsIgnoreCase(typedTicket.getType())) { + return getTable(typedTicket, "get a table"); + } else if (JsVariableType.FIGURE.equalsIgnoreCase(typedTicket.getType())) { + return getFigure(typedTicket); + } else if (JsVariableType.PANDAS.equalsIgnoreCase(typedTicket.getType())) { + return getPandasTable(typedTicket); + } else if (JsVariableType.PARTITIONEDTABLE.equalsIgnoreCase(typedTicket.getType())) { + return getPartitionedTable(typedTicket); + } else if (JsVariableType.HIERARCHICALTABLE.equalsIgnoreCase(typedTicket.getType())) { + return getHierarchicalTable(typedTicket); + } else { + if (JsVariableType.TABLEMAP.equalsIgnoreCase(typedTicket.getType())) { + JsLog.warn( + "TableMap is now known as PartitionedTable, fetching as a plain widget. To fetch as a PartitionedTable use that as the type."); + } + if (JsVariableType.TREETABLE.equalsIgnoreCase(typedTicket.getType())) { + JsLog.warn( + "TreeTable is now HierarchicalTable, fetching as a plain widget. To fetch as a HierarchicalTable use that as this type."); + } + return getWidget(typedTicket, true); + } } + /** + * Retrieve an object from it's definition + * + * @param definition Variable definition to get + * @return The object requested + */ public Promise getObject(JsVariableDefinition definition) { if (JsVariableType.TABLE.equalsIgnoreCase(definition.getType())) { return getTable(definition, null); @@ -911,58 +963,91 @@ private TicketAndPromise exportScopeTicket(JsVariableDefinition varDef) { public Promise getPartitionedTable(JsVariableDefinition varDef) { return whenServerReady("get a partitioned table") - .then(server -> getWidget(varDef)) + .then(ignore -> getWidget(varDef, false)) .then(widget -> new JsPartitionedTable(this, widget).refetch()); } - public Promise getTreeTable(JsVariableDefinition varDef) { - return getWidget(varDef).then(w -> Promise.resolve(new JsTreeTable(this, w))); + public Promise getPartitionedTable(TypedTicket ticket) { + return whenServerReady("get a partitioned table") + .then(ignore -> new JsPartitionedTable(this, new JsWidget(this, ticket)).refetch()); } public Promise getHierarchicalTable(JsVariableDefinition varDef) { - return getWidget(varDef).then(w -> Promise.resolve(new JsTreeTable(this, w))); + return whenServerReady("get a hierarchical table") + .then(ignore -> getWidget(varDef)) + .then(widget -> Promise.resolve(new JsTreeTable(this, widget))); + } + + public Promise getHierarchicalTable(TypedTicket typedTicket) { + return whenServerReady("get a hierarchical table") + .then(ignore -> getWidget(typedTicket, true)) + .then(widget -> Promise.resolve(new JsTreeTable(this, widget))); } public Promise getFigure(JsVariableDefinition varDef) { - if (!varDef.getType().equalsIgnoreCase("Figure")) { - throw new IllegalArgumentException("Can't load as a figure: " + varDef.getType()); - } return whenServerReady("get a figure") .then(server -> new JsFigure(this, - c -> { - getWidget(varDef).then(widget -> { - FetchObjectResponse legacyResponse = new FetchObjectResponse(); - legacyResponse.setData(widget.getDataAsU8()); - legacyResponse.setType(widget.getType()); - legacyResponse.setTypedExportIdsList(Arrays.stream(widget.getExportedObjects()) - .map(JsWidgetExportedObject::typedTicket).toArray(TypedTicket[]::new)); - c.apply(null, legacyResponse); - widget.close(); - return null; - }, error -> { - c.apply(error, null); - return null; - }); - }).refetch()); + c -> getWidget(varDef).then(widget -> { + FetchObjectResponse legacyResponse = new FetchObjectResponse(); + legacyResponse.setData(widget.getDataAsU8()); + legacyResponse.setType(widget.getType()); + legacyResponse.setTypedExportIdsList( + Arrays.stream(widget.getExportedObjects()).map(JsWidgetExportedObject::takeTicket) + .toArray(TypedTicket[]::new)); + c.apply(null, legacyResponse); + widget.close(); + return null; + }, error -> { + c.apply(error, null); + return null; + })).refetch()); + } + + public Promise getFigure(TypedTicket ticket) { + return whenServerReady("get a figure") + .then(ignore -> new JsFigure(this, + c -> getWidget(ticket, true).then(widget -> { + FetchObjectResponse legacyResponse = new FetchObjectResponse(); + legacyResponse.setData(widget.getDataAsU8()); + legacyResponse.setType(widget.getType()); + legacyResponse.setTypedExportIdsList( + Arrays.stream(widget.getExportedObjects()).map(JsWidgetExportedObject::takeTicket) + .toArray(TypedTicket[]::new)); + c.apply(null, legacyResponse); + widget.close(); + return null; + }, error -> { + c.apply(error, null); + return null; + })).refetch()); + } + + /** + * Gets a Pandas Table from the Pandas table widget provided. Closes the widget when done. + * + * @param widget Pandas table widget + * @return Table representing the Pandas data frame + */ + private Promise getPandasTable(JsWidget widget) { + TypedTicket typedTicket = widget.getExportedObjects()[0].takeTicket(); + + // We don't need to keep the original widget open, we can just close it right away + widget.close(); + + return getTable(typedTicket, "table for pandas").then(table -> { + // TODO(deephaven-core#3604) if using a new session don't attempt a reconnect + // never attempt a reconnect, since we might have a different widget schema entirely + table.addEventListener(JsTable.EVENT_DISCONNECT, ignore -> table.close()); + return Promise.resolve(table); + }); } public Promise getPandasTable(JsVariableDefinition definition) { - return getWidget(definition) - .then(widget -> { - return Callbacks.grpcUnaryPromise(c -> { - tableServiceClient().getExportedTableCreationResponse(widget.getExportedObjects()[0].typedTicket().getTicket(), metadata(), c::apply); - }).then(etcr -> { - ClientTableState cts = newStateFromUnsolicitedTable(etcr, "table for pandas"); - JsTable table = new JsTable(this, cts); - // TODO(deephaven-core#3604) if using a new session don't attempt a reconnect - // never attempt a reconnect, since we might have a different widget schema entirely - table.addEventListener(JsTable.EVENT_DISCONNECT, ignore -> table.close()); - return Promise.resolve(table); - }).then(table -> { - widget.close(); - return Promise.resolve(table); - }); - }); + return getWidget(definition).then(this::getPandasTable); + } + + public Promise getPandasTable(TypedTicket ticket) { + return getWidget(ticket, true).then(this::getPandasTable); } private TypedTicket createTypedTicket(JsVariableDefinition varDef) { @@ -972,19 +1057,53 @@ private TypedTicket createTypedTicket(JsVariableDefinition varDef) { return typedTicket; } + + /** + * Retrieve a widget using a variable definition. Creates a new export ticket to reference the variable. + * Intentionally races the ticket export with the variable export to parallelize the requests. + * + * @param varDef Variable definition to get the widget for + * @return Promise of the widget + */ public Promise getWidget(JsVariableDefinition varDef) { + return getWidget(varDef, true); + } + + /** + * Retrieve a widget using a variable definition. Creates a new export ticket to reference the variable. + * Intentionally races the ticket export with the variable export to parallelize the requests. + * + * @param varDef Variable definition to get the widget for + * @param refetch Whether to refetch the widget before resolving or not + * @return Promise of the widget + */ + public Promise getWidget(JsVariableDefinition varDef, boolean refetch) { return exportScopeTicket(varDef) .race(ticket -> { TypedTicket typedTicket = new TypedTicket(); typedTicket.setType(varDef.getType()); typedTicket.setTicket(ticket); - return getWidget(typedTicket); + return getWidget(typedTicket, refetch); }).promise(); } - public Promise getWidget(TypedTicket typedTicket) { + /** + * Retrieve a widget using a ticket. Does not create a new ticket. + * + * @param typedTicket Ticket to fetch + * @param refetch Whether to refetch the widget. If false, you must ensure you call {@JsWidget#refetch()} before + * using the widget. + * @return Promise of the widget + */ + public Promise getWidget(TypedTicket typedTicket, boolean refetch) { return whenServerReady("get a widget") - .then(response -> new JsWidget(this, typedTicket).refetch()); + .then(response -> Promise.resolve(new JsWidget(this, typedTicket))) + .then(widget -> { + if (refetch) { + return widget.refetch(); + } + return Promise.resolve(widget); + }); } public void registerSimpleReconnectable(HasLifecycle figure) { diff --git a/web/client-api/src/main/java/io/deephaven/web/client/api/widget/JsWidget.java b/web/client-api/src/main/java/io/deephaven/web/client/api/widget/JsWidget.java index 986a0111ef1..b83a073ceda 100644 --- a/web/client-api/src/main/java/io/deephaven/web/client/api/widget/JsWidget.java +++ b/web/client-api/src/main/java/io/deephaven/web/client/api/widget/JsWidget.java @@ -91,7 +91,7 @@ private void closeStream() { private void closeExportedObjects() { for (int i = 0; i < exportedObjects.length; i++) { JsWidgetExportedObject exportedObject = exportedObjects.getAt(i); - if (exportedObject.isTicketOwner()) { + if (exportedObject.isOwner()) { exportedObject.close(); } } @@ -115,7 +115,6 @@ public Promise refetch() { messageStream = streamFactory.get(); messageStream.onData(res -> { - JsArray responseObjects = res.getData().getExportedReferencesList() .map((p0, p1, p2) -> new JsWidgetExportedObject(connection, p0)); if (!hasFetched) { diff --git a/web/client-api/src/main/java/io/deephaven/web/client/api/widget/JsWidgetExportedObject.java b/web/client-api/src/main/java/io/deephaven/web/client/api/widget/JsWidgetExportedObject.java index 31ef2944c63..00b7a2a4c2a 100644 --- a/web/client-api/src/main/java/io/deephaven/web/client/api/widget/JsWidgetExportedObject.java +++ b/web/client-api/src/main/java/io/deephaven/web/client/api/widget/JsWidgetExportedObject.java @@ -26,7 +26,7 @@ public class JsWidgetExportedObject implements ServerObject { private final TypedTicket ticket; /** Whether this exported object instance is the owner of the ticket or not. */ - private final boolean isTicketOwner = true; + private boolean isOwner = true; public JsWidgetExportedObject(WorkerConnection connection, TypedTicket ticket) { this.connection = connection; @@ -46,16 +46,32 @@ public TypedTicket typedTicket() { return typedTicket; } - public boolean isTicketOwner() { - return isTicketOwner; + public boolean isOwner() { + return isOwner; } public Promise fetch() { return fetch(false); } + private void verifyIsOwner() { + if (!isOwner) { + throw new IllegalStateException("Exported object is no longer the owner of this ticket."); + } + } + + /** + * Takes ownership of this exported object. Returns the ticket for this object. + */ + public TypedTicket takeTicket() { + verifyIsOwner(); + isOwner = false; + return typedTicket(); + } + /** * Fetches the object from the server. + * * @param takeOwnership Whether to take ownership of the object. Defaults to false. * * @return a promise that resolves to the object @@ -66,11 +82,10 @@ public Promise fetch(@JsOptional Boolean takeOwnership) { takeOwnership = false; } if (takeOwnership) { - // TODO: if takeOwnership, don't want to create a new exportScopeTicket... - isTicketOwner = false; - throw new RuntimeException("Haven't implemented this yet"); + return this.connection.getObject(takeTicket()); } - return this.connection.getObject(ticket); + return this.connection.getObject( + new JsVariableDefinition(ticket.getType(), null, ticket.getTicket().getTicket_asB64(), null)); } /** @@ -79,9 +94,7 @@ public Promise fetch(@JsOptional Boolean takeOwnership) { */ @JsMethod public void close() { - if (!isTicketOwner) { - throw new IllegalStateException("Exported object is no longer the owner of this ticket."); - } + verifyIsOwner(); connection.releaseTicket(ticket.getTicket()); } } diff --git a/web/client-api/src/main/java/io/deephaven/web/client/ide/IdeSession.java b/web/client-api/src/main/java/io/deephaven/web/client/ide/IdeSession.java index 6d8e1b69cb0..75bedece3c8 100644 --- a/web/client-api/src/main/java/io/deephaven/web/client/ide/IdeSession.java +++ b/web/client-api/src/main/java/io/deephaven/web/client/ide/IdeSession.java @@ -130,12 +130,12 @@ public Promise getFigure(String name) { */ public Promise getTreeTable(String name) { return connection.getVariableDefinition(name, JsVariableType.HIERARCHICALTABLE) - .then(connection::getTreeTable); + .then(connection::getHierarchicalTable); } public Promise getHierarchicalTable(String name) { return connection.getVariableDefinition(name, JsVariableType.HIERARCHICALTABLE) - .then(connection::getTreeTable); + .then(connection::getHierarchicalTable); } public Promise getObject(@TsTypeRef(JsVariableDescriptor.class) JsPropertyMap definitionObject) { From 732938a840503b4434dbe3434cec5e3434a61c2d Mon Sep 17 00:00:00 2001 From: Mike Bender Date: Sat, 2 Dec 2023 00:37:15 -0500 Subject: [PATCH 06/20] Clean up a bit more based on self review --- .../web/client/api/JsPartitionedTable.java | 9 +-- .../web/client/api/WorkerConnection.java | 58 ++++++++----------- 2 files changed, 25 insertions(+), 42 deletions(-) diff --git a/web/client-api/src/main/java/io/deephaven/web/client/api/JsPartitionedTable.java b/web/client-api/src/main/java/io/deephaven/web/client/api/JsPartitionedTable.java index 44ccb4e0a31..789d7d25407 100644 --- a/web/client-api/src/main/java/io/deephaven/web/client/api/JsPartitionedTable.java +++ b/web/client-api/src/main/java/io/deephaven/web/client/api/JsPartitionedTable.java @@ -108,14 +108,7 @@ public Promise refetch() { this.columns = JsObject.freeze(columns); this.keyColumns = JsObject.freeze(keyColumns); - - Promise promise = w.getExportedObjects()[0].fetch(true); - promise.then(ignore -> { - // We only need to keep the widget open long enough to get the exported objects - w.close(); - return null; - }); - return promise; + return w.getExportedObjects()[0].fetch(true); }).then(result -> { keys = (JsTable) result; // TODO(deephaven-core#3604) in case of a new session, we should do a full refetch diff --git a/web/client-api/src/main/java/io/deephaven/web/client/api/WorkerConnection.java b/web/client-api/src/main/java/io/deephaven/web/client/api/WorkerConnection.java index 0d7d23b493d..2909935affe 100644 --- a/web/client-api/src/main/java/io/deephaven/web/client/api/WorkerConnection.java +++ b/web/client-api/src/main/java/io/deephaven/web/client/api/WorkerConnection.java @@ -767,8 +767,6 @@ public Promise getTable(JsVariableDefinition varDef, @Nullable Boolean * @return Promise for the table */ public Promise getTable(TypedTicket ticket, String fetchSummary) { - // Note that creating a CTS like this means we can't actually refetch it, but that's okay, we can't - // reconnect in this way without refetching the entire figure anyway. return Callbacks.grpcUnaryPromise(c -> { tableServiceClient().getExportedTableCreationResponse(ticket.getTicket(), metadata(), @@ -984,42 +982,34 @@ public Promise getHierarchicalTable(TypedTicket typedTicket) { .then(widget -> Promise.resolve(new JsTreeTable(this, widget))); } + private JsFigure.FigureFetch getWidgetFigureFetch(Promise widgetPromise) { + return c -> widgetPromise.then(widget -> { + FetchObjectResponse legacyResponse = new FetchObjectResponse(); + legacyResponse.setData(widget.getDataAsU8()); + legacyResponse.setType(widget.getType()); + legacyResponse.setTypedExportIdsList( + Arrays.stream(widget.getExportedObjects()).map(JsWidgetExportedObject::takeTicket) + .toArray(TypedTicket[]::new)); + c.apply(null, legacyResponse); + widget.close(); + return null; + }, error -> { + c.apply(error, null); + return null; + }); + } + + private Promise getFigure(Promise widgetPromise) { + return whenServerReady("get a figure").then(server -> new JsFigure(this, getWidgetFigureFetch(widgetPromise)).refetch()); + + } + public Promise getFigure(JsVariableDefinition varDef) { - return whenServerReady("get a figure") - .then(server -> new JsFigure(this, - c -> getWidget(varDef).then(widget -> { - FetchObjectResponse legacyResponse = new FetchObjectResponse(); - legacyResponse.setData(widget.getDataAsU8()); - legacyResponse.setType(widget.getType()); - legacyResponse.setTypedExportIdsList( - Arrays.stream(widget.getExportedObjects()).map(JsWidgetExportedObject::takeTicket) - .toArray(TypedTicket[]::new)); - c.apply(null, legacyResponse); - widget.close(); - return null; - }, error -> { - c.apply(error, null); - return null; - })).refetch()); + return getFigure(getWidget(varDef)); } public Promise getFigure(TypedTicket ticket) { - return whenServerReady("get a figure") - .then(ignore -> new JsFigure(this, - c -> getWidget(ticket, true).then(widget -> { - FetchObjectResponse legacyResponse = new FetchObjectResponse(); - legacyResponse.setData(widget.getDataAsU8()); - legacyResponse.setType(widget.getType()); - legacyResponse.setTypedExportIdsList( - Arrays.stream(widget.getExportedObjects()).map(JsWidgetExportedObject::takeTicket) - .toArray(TypedTicket[]::new)); - c.apply(null, legacyResponse); - widget.close(); - return null; - }, error -> { - c.apply(error, null); - return null; - })).refetch()); + return getFigure(getWidget(ticket, true)); } /** From 91e9adfb0b71a8812e2e20e2e592610373e93d9b Mon Sep 17 00:00:00 2001 From: mikebender Date: Mon, 4 Dec 2023 10:22:26 -0500 Subject: [PATCH 07/20] Apply spotless --- .../java/io/deephaven/web/client/api/WorkerConnection.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/web/client-api/src/main/java/io/deephaven/web/client/api/WorkerConnection.java b/web/client-api/src/main/java/io/deephaven/web/client/api/WorkerConnection.java index 2909935affe..5bd2d2f0afc 100644 --- a/web/client-api/src/main/java/io/deephaven/web/client/api/WorkerConnection.java +++ b/web/client-api/src/main/java/io/deephaven/web/client/api/WorkerConnection.java @@ -1000,7 +1000,8 @@ private JsFigure.FigureFetch getWidgetFigureFetch(Promise widgetPromis } private Promise getFigure(Promise widgetPromise) { - return whenServerReady("get a figure").then(server -> new JsFigure(this, getWidgetFigureFetch(widgetPromise)).refetch()); + return whenServerReady("get a figure") + .then(server -> new JsFigure(this, getWidgetFigureFetch(widgetPromise)).refetch()); } From 108936afcf35135e531afda62dab529a43f2dffb Mon Sep 17 00:00:00 2001 From: Mike Bender Date: Mon, 4 Dec 2023 14:02:20 -0500 Subject: [PATCH 08/20] Update web/client-api/src/main/java/io/deephaven/web/client/api/WorkerConnection.java Co-authored-by: Colin Alworth --- .../main/java/io/deephaven/web/client/api/WorkerConnection.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/web/client-api/src/main/java/io/deephaven/web/client/api/WorkerConnection.java b/web/client-api/src/main/java/io/deephaven/web/client/api/WorkerConnection.java index 5bd2d2f0afc..0181dec56a6 100644 --- a/web/client-api/src/main/java/io/deephaven/web/client/api/WorkerConnection.java +++ b/web/client-api/src/main/java/io/deephaven/web/client/api/WorkerConnection.java @@ -779,7 +779,7 @@ public Promise getTable(TypedTicket ticket, String fetchSummary) { } /** - * Get an object from it's ticket + * Get an object from its ticket. * * @param typedTicket Ticket to get the object for * @return The object requested From cbc0ae3396cb162b42b8337463aa11d19790cd06 Mon Sep 17 00:00:00 2001 From: Mike Bender Date: Mon, 4 Dec 2023 14:05:35 -0500 Subject: [PATCH 09/20] Apply suggestions from code review Co-authored-by: Colin Alworth --- .../java/io/deephaven/web/client/api/WorkerConnection.java | 4 ++-- .../web/client/api/widget/JsWidgetExportedObject.java | 1 - 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/web/client-api/src/main/java/io/deephaven/web/client/api/WorkerConnection.java b/web/client-api/src/main/java/io/deephaven/web/client/api/WorkerConnection.java index 0181dec56a6..c2c3841da13 100644 --- a/web/client-api/src/main/java/io/deephaven/web/client/api/WorkerConnection.java +++ b/web/client-api/src/main/java/io/deephaven/web/client/api/WorkerConnection.java @@ -760,7 +760,7 @@ public Promise getTable(JsVariableDefinition varDef, @Nullable Boolean } /** - * Export a table from the provided ticket + * Export a table from the provided ticket. * * @param ticket Ticket of the table to export * @param fetchSummary Description for the fetch. Used for logging and errors. @@ -809,7 +809,7 @@ public Promise getObject(TypedTicket typedTicket) { } /** - * Retrieve an object from it's definition + * Retrieve an object from its definition. * * @param definition Variable definition to get * @return The object requested diff --git a/web/client-api/src/main/java/io/deephaven/web/client/api/widget/JsWidgetExportedObject.java b/web/client-api/src/main/java/io/deephaven/web/client/api/widget/JsWidgetExportedObject.java index 00b7a2a4c2a..215a7775bde 100644 --- a/web/client-api/src/main/java/io/deephaven/web/client/api/widget/JsWidgetExportedObject.java +++ b/web/client-api/src/main/java/io/deephaven/web/client/api/widget/JsWidgetExportedObject.java @@ -73,7 +73,6 @@ public TypedTicket takeTicket() { * Fetches the object from the server. * * @param takeOwnership Whether to take ownership of the object. Defaults to false. - * * @return a promise that resolves to the object */ @JsMethod From ee5ef0366277fb769d25b6374d36a1c2df67eade Mon Sep 17 00:00:00 2001 From: mikebender Date: Mon, 4 Dec 2023 14:05:51 -0500 Subject: [PATCH 10/20] Clean up from code review --- .../java/io/deephaven/web/client/api/WorkerConnection.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/web/client-api/src/main/java/io/deephaven/web/client/api/WorkerConnection.java b/web/client-api/src/main/java/io/deephaven/web/client/api/WorkerConnection.java index c2c3841da13..8dd74b2a0a9 100644 --- a/web/client-api/src/main/java/io/deephaven/web/client/api/WorkerConnection.java +++ b/web/client-api/src/main/java/io/deephaven/web/client/api/WorkerConnection.java @@ -1088,8 +1088,8 @@ public Promise getWidget(JsVariableDefinition varDef, boolean refetch) */ public Promise getWidget(TypedTicket typedTicket, boolean refetch) { return whenServerReady("get a widget") - .then(response -> Promise.resolve(new JsWidget(this, typedTicket))) - .then(widget -> { + .then(response -> { + JsWidget widget = new JsWidget(this, typedTicket); if (refetch) { return widget.refetch(); } From 8532cd15610734b00dc8bf86afbf5e75bfcf1098 Mon Sep 17 00:00:00 2001 From: mikebender Date: Mon, 4 Dec 2023 14:30:27 -0500 Subject: [PATCH 11/20] Always verify the ownership when fetching exported object --- .../main/java/io/deephaven/web/client/api/WorkerConnection.java | 2 +- .../deephaven/web/client/api/widget/JsWidgetExportedObject.java | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/web/client-api/src/main/java/io/deephaven/web/client/api/WorkerConnection.java b/web/client-api/src/main/java/io/deephaven/web/client/api/WorkerConnection.java index 8dd74b2a0a9..416bfa86f18 100644 --- a/web/client-api/src/main/java/io/deephaven/web/client/api/WorkerConnection.java +++ b/web/client-api/src/main/java/io/deephaven/web/client/api/WorkerConnection.java @@ -760,7 +760,7 @@ public Promise getTable(JsVariableDefinition varDef, @Nullable Boolean } /** - * Export a table from the provided ticket. + * Export a table from the provided ticket. Intended for server-side exports only. * * @param ticket Ticket of the table to export * @param fetchSummary Description for the fetch. Used for logging and errors. diff --git a/web/client-api/src/main/java/io/deephaven/web/client/api/widget/JsWidgetExportedObject.java b/web/client-api/src/main/java/io/deephaven/web/client/api/widget/JsWidgetExportedObject.java index 215a7775bde..2a0858e112e 100644 --- a/web/client-api/src/main/java/io/deephaven/web/client/api/widget/JsWidgetExportedObject.java +++ b/web/client-api/src/main/java/io/deephaven/web/client/api/widget/JsWidgetExportedObject.java @@ -80,6 +80,7 @@ public Promise fetch(@JsOptional Boolean takeOwnership) { if (takeOwnership == null) { takeOwnership = false; } + verifyIsOwner(); if (takeOwnership) { return this.connection.getObject(takeTicket()); } From ba041a5dedf798ceb0bd0114eb31db3564682386 Mon Sep 17 00:00:00 2001 From: mikebender Date: Thu, 7 Dec 2023 17:32:36 -0500 Subject: [PATCH 12/20] Changes from code review --- .../java/io/deephaven/web/client/api/widget/JsWidget.java | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/web/client-api/src/main/java/io/deephaven/web/client/api/widget/JsWidget.java b/web/client-api/src/main/java/io/deephaven/web/client/api/widget/JsWidget.java index b83a073ceda..2cc6ff8ea59 100644 --- a/web/client-api/src/main/java/io/deephaven/web/client/api/widget/JsWidget.java +++ b/web/client-api/src/main/java/io/deephaven/web/client/api/widget/JsWidget.java @@ -132,9 +132,10 @@ public Promise refetch() { messageStream.onStatus(status -> { if (!status.isOk()) { reject.onInvoke(status.getDetails()); - fireEvent(EVENT_CLOSE); - closeStream(); } + fireEvent(EVENT_CLOSE); + closeStream(); + connection.releaseTicket(getTicket()); }); messageStream.onEnd(status -> { closeStream(); From 59085d91e8d56c22a2a6c995bf5619f7ea7a647c Mon Sep 17 00:00:00 2001 From: mikebender Date: Thu, 7 Dec 2023 23:32:03 -0500 Subject: [PATCH 13/20] Cleanup after review - Can release the ticket of a widget automatically if the server closes the object - no way to reconnect (currently) - Remove unnecessary closes --- .../java/io/deephaven/web/client/api/JsPartitionedTable.java | 2 -- .../java/io/deephaven/web/client/api/WorkerConnection.java | 5 ----- .../java/io/deephaven/web/client/api/widget/JsWidget.java | 3 +++ 3 files changed, 3 insertions(+), 7 deletions(-) diff --git a/web/client-api/src/main/java/io/deephaven/web/client/api/JsPartitionedTable.java b/web/client-api/src/main/java/io/deephaven/web/client/api/JsPartitionedTable.java index 789d7d25407..798b4b9f88f 100644 --- a/web/client-api/src/main/java/io/deephaven/web/client/api/JsPartitionedTable.java +++ b/web/client-api/src/main/java/io/deephaven/web/client/api/JsPartitionedTable.java @@ -308,8 +308,6 @@ public Promise getKeyTable() { * will not affect tables in use. */ public void close() { - widget.close(); - if (keys != null) { keys.close(); } diff --git a/web/client-api/src/main/java/io/deephaven/web/client/api/WorkerConnection.java b/web/client-api/src/main/java/io/deephaven/web/client/api/WorkerConnection.java index 416bfa86f18..9fbbad9b1a9 100644 --- a/web/client-api/src/main/java/io/deephaven/web/client/api/WorkerConnection.java +++ b/web/client-api/src/main/java/io/deephaven/web/client/api/WorkerConnection.java @@ -991,7 +991,6 @@ private JsFigure.FigureFetch getWidgetFigureFetch(Promise widgetPromis Arrays.stream(widget.getExportedObjects()).map(JsWidgetExportedObject::takeTicket) .toArray(TypedTicket[]::new)); c.apply(null, legacyResponse); - widget.close(); return null; }, error -> { c.apply(error, null); @@ -1021,10 +1020,6 @@ public Promise getFigure(TypedTicket ticket) { */ private Promise getPandasTable(JsWidget widget) { TypedTicket typedTicket = widget.getExportedObjects()[0].takeTicket(); - - // We don't need to keep the original widget open, we can just close it right away - widget.close(); - return getTable(typedTicket, "table for pandas").then(table -> { // TODO(deephaven-core#3604) if using a new session don't attempt a reconnect // never attempt a reconnect, since we might have a different widget schema entirely diff --git a/web/client-api/src/main/java/io/deephaven/web/client/api/widget/JsWidget.java b/web/client-api/src/main/java/io/deephaven/web/client/api/widget/JsWidget.java index 2cc6ff8ea59..2845de28fbd 100644 --- a/web/client-api/src/main/java/io/deephaven/web/client/api/widget/JsWidget.java +++ b/web/client-api/src/main/java/io/deephaven/web/client/api/widget/JsWidget.java @@ -135,6 +135,9 @@ public Promise refetch() { } fireEvent(EVENT_CLOSE); closeStream(); + + // We can release the ticket here because the widget can no longer be used at this point after the server has closed it. + // As a result, fetch-only widgets do not need to be closed, as the server closes them right away. connection.releaseTicket(getTicket()); }); messageStream.onEnd(status -> { From 81ec321aa9bf66ccb335ca9ef9236e2ae1f0201c Mon Sep 17 00:00:00 2001 From: mikebender Date: Thu, 7 Dec 2023 23:34:28 -0500 Subject: [PATCH 14/20] Remove unnecessary ticket release --- .../main/java/io/deephaven/web/client/api/tree/JsTreeTable.java | 2 -- 1 file changed, 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 c6ffa93cbd3..4f7bd28067f 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 @@ -979,8 +979,6 @@ public void close() { connection.unregisterSimpleReconnectable(this); - connection.releaseTicket(widget.getTicket()); - if (filteredTable != null) { filteredTable.release(); filteredTable = null; From c9e36c9ca4311bc7f190dd3011977c88aa566984 Mon Sep 17 00:00:00 2001 From: mikebender Date: Fri, 8 Dec 2023 09:15:01 -0500 Subject: [PATCH 15/20] Spotless --- .../main/java/io/deephaven/web/client/api/widget/JsWidget.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/web/client-api/src/main/java/io/deephaven/web/client/api/widget/JsWidget.java b/web/client-api/src/main/java/io/deephaven/web/client/api/widget/JsWidget.java index 2845de28fbd..0e466393e59 100644 --- a/web/client-api/src/main/java/io/deephaven/web/client/api/widget/JsWidget.java +++ b/web/client-api/src/main/java/io/deephaven/web/client/api/widget/JsWidget.java @@ -136,7 +136,8 @@ public Promise refetch() { fireEvent(EVENT_CLOSE); closeStream(); - // We can release the ticket here because the widget can no longer be used at this point after the server has closed it. + // We can release the ticket here because the widget can no longer be used at this point after the + // server has closed it. // As a result, fetch-only widgets do not need to be closed, as the server closes them right away. connection.releaseTicket(getTicket()); }); From 73172d38669278755624b40d2e4658ac87332133 Mon Sep 17 00:00:00 2001 From: Mike Bender Date: Fri, 8 Dec 2023 12:47:50 -0500 Subject: [PATCH 16/20] Update web/client-api/src/main/java/io/deephaven/web/client/api/WorkerConnection.java Co-authored-by: Colin Alworth --- .../main/java/io/deephaven/web/client/api/WorkerConnection.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/web/client-api/src/main/java/io/deephaven/web/client/api/WorkerConnection.java b/web/client-api/src/main/java/io/deephaven/web/client/api/WorkerConnection.java index 9fbbad9b1a9..f36f954bbe3 100644 --- a/web/client-api/src/main/java/io/deephaven/web/client/api/WorkerConnection.java +++ b/web/client-api/src/main/java/io/deephaven/web/client/api/WorkerConnection.java @@ -1077,7 +1077,7 @@ public Promise getWidget(JsVariableDefinition varDef, boolean refetch) * Retrieve a widget using a ticket. Does not create a new ticket. * * @param typedTicket Ticket to fetch - * @param refetch Whether to refetch the widget. If false, you must ensure you call {@JsWidget#refetch()} before + * @param refetch Whether to refetch the widget. If false, you must ensure you call {@link JsWidget#refetch()} before * using the widget. * @return Promise of the widget */ From a1add7fed8adc37b86bd01065655f99ac2c7f3ed Mon Sep 17 00:00:00 2001 From: Mike Bender Date: Fri, 8 Dec 2023 12:51:05 -0500 Subject: [PATCH 17/20] Update web/client-api/src/main/java/io/deephaven/web/client/api/widget/JsWidget.java Co-authored-by: Colin Alworth --- .../main/java/io/deephaven/web/client/api/widget/JsWidget.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/web/client-api/src/main/java/io/deephaven/web/client/api/widget/JsWidget.java b/web/client-api/src/main/java/io/deephaven/web/client/api/widget/JsWidget.java index 0e466393e59..91ed0f6958f 100644 --- a/web/client-api/src/main/java/io/deephaven/web/client/api/widget/JsWidget.java +++ b/web/client-api/src/main/java/io/deephaven/web/client/api/widget/JsWidget.java @@ -86,7 +86,7 @@ private void closeStream() { } /** - * Also closes all the exported objects that the widget still owns + * Closes all the exported objects that the widget still owns. */ private void closeExportedObjects() { for (int i = 0; i < exportedObjects.length; i++) { From 4b27d1374b16e52933801428f0b4dbf4439b244f Mon Sep 17 00:00:00 2001 From: mikebender Date: Fri, 8 Dec 2023 13:06:51 -0500 Subject: [PATCH 18/20] Clean up based on review - Made many methods private instead of public - Renamed methods to `getExported*` to make it clear they are for server-side exports - Changed default method of `JsWidgetExportedObject.fetch` so this is not a breaking change --- .../web/client/api/WorkerConnection.java | 50 +++++++++---------- .../api/widget/JsWidgetExportedObject.java | 6 +-- 2 files changed, 28 insertions(+), 28 deletions(-) diff --git a/web/client-api/src/main/java/io/deephaven/web/client/api/WorkerConnection.java b/web/client-api/src/main/java/io/deephaven/web/client/api/WorkerConnection.java index f36f954bbe3..2ec3f76ba2a 100644 --- a/web/client-api/src/main/java/io/deephaven/web/client/api/WorkerConnection.java +++ b/web/client-api/src/main/java/io/deephaven/web/client/api/WorkerConnection.java @@ -766,7 +766,7 @@ public Promise getTable(JsVariableDefinition varDef, @Nullable Boolean * @param fetchSummary Description for the fetch. Used for logging and errors. * @return Promise for the table */ - public Promise getTable(TypedTicket ticket, String fetchSummary) { + private Promise getExportedTable(TypedTicket ticket, String fetchSummary) { return Callbacks.grpcUnaryPromise(c -> { tableServiceClient().getExportedTableCreationResponse(ticket.getTicket(), metadata(), @@ -779,22 +779,22 @@ public Promise getTable(TypedTicket ticket, String fetchSummary) { } /** - * Get an object from its ticket. + * Get an object from its server-side exported ticket. * * @param typedTicket Ticket to get the object for * @return The object requested */ - public Promise getObject(TypedTicket typedTicket) { + public Promise getExportedObject(TypedTicket typedTicket) { if (JsVariableType.TABLE.equalsIgnoreCase(typedTicket.getType())) { - return getTable(typedTicket, "get a table"); + return getExportedTable(typedTicket, "get a table"); } else if (JsVariableType.FIGURE.equalsIgnoreCase(typedTicket.getType())) { - return getFigure(typedTicket); + return getExportedFigure(typedTicket); } else if (JsVariableType.PANDAS.equalsIgnoreCase(typedTicket.getType())) { - return getPandasTable(typedTicket); + return getExportedPandasTable(typedTicket); } else if (JsVariableType.PARTITIONEDTABLE.equalsIgnoreCase(typedTicket.getType())) { - return getPartitionedTable(typedTicket); + return getExportedPartitionedTable(typedTicket); } else if (JsVariableType.HIERARCHICALTABLE.equalsIgnoreCase(typedTicket.getType())) { - return getHierarchicalTable(typedTicket); + return getExportedHierarchicalTable(typedTicket); } else { if (JsVariableType.TABLEMAP.equalsIgnoreCase(typedTicket.getType())) { JsLog.warn( @@ -804,7 +804,7 @@ public Promise getObject(TypedTicket typedTicket) { JsLog.warn( "TreeTable is now HierarchicalTable, fetching as a plain widget. To fetch as a HierarchicalTable use that as this type."); } - return getWidget(typedTicket, true); + return getExportedWidget(typedTicket, true); } } @@ -965,7 +965,7 @@ public Promise getPartitionedTable(JsVariableDefinition varD .then(widget -> new JsPartitionedTable(this, widget).refetch()); } - public Promise getPartitionedTable(TypedTicket ticket) { + private Promise getExportedPartitionedTable(TypedTicket ticket) { return whenServerReady("get a partitioned table") .then(ignore -> new JsPartitionedTable(this, new JsWidget(this, ticket)).refetch()); } @@ -976,9 +976,9 @@ public Promise getHierarchicalTable(JsVariableDefinition varDef) { .then(widget -> Promise.resolve(new JsTreeTable(this, widget))); } - public Promise getHierarchicalTable(TypedTicket typedTicket) { + private Promise getExportedHierarchicalTable(TypedTicket typedTicket) { return whenServerReady("get a hierarchical table") - .then(ignore -> getWidget(typedTicket, true)) + .then(ignore -> getExportedWidget(typedTicket, true)) .then(widget -> Promise.resolve(new JsTreeTable(this, widget))); } @@ -1008,8 +1008,8 @@ public Promise getFigure(JsVariableDefinition varDef) { return getFigure(getWidget(varDef)); } - public Promise getFigure(TypedTicket ticket) { - return getFigure(getWidget(ticket, true)); + private Promise getExportedFigure(TypedTicket ticket) { + return getFigure(getExportedWidget(ticket, true)); } /** @@ -1018,9 +1018,9 @@ public Promise getFigure(TypedTicket ticket) { * @param widget Pandas table widget * @return Table representing the Pandas data frame */ - private Promise getPandasTable(JsWidget widget) { + private Promise getWidgetPandasTable(JsWidget widget) { TypedTicket typedTicket = widget.getExportedObjects()[0].takeTicket(); - return getTable(typedTicket, "table for pandas").then(table -> { + return getExportedTable(typedTicket, "table for pandas").then(table -> { // TODO(deephaven-core#3604) if using a new session don't attempt a reconnect // never attempt a reconnect, since we might have a different widget schema entirely table.addEventListener(JsTable.EVENT_DISCONNECT, ignore -> table.close()); @@ -1029,11 +1029,11 @@ private Promise getPandasTable(JsWidget widget) { } public Promise getPandasTable(JsVariableDefinition definition) { - return getWidget(definition).then(this::getPandasTable); + return getWidget(definition).then(this::getWidgetPandasTable); } - public Promise getPandasTable(TypedTicket ticket) { - return getWidget(ticket, true).then(this::getPandasTable); + private Promise getExportedPandasTable(TypedTicket ticket) { + return getExportedWidget(ticket, true).then(this::getWidgetPandasTable); } private TypedTicket createTypedTicket(JsVariableDefinition varDef) { @@ -1060,16 +1060,16 @@ public Promise getWidget(JsVariableDefinition varDef) { * Intentionally races the ticket export with the variable export to parallelize the requests. * * @param varDef Variable definition to get the widget for - * @param refetch Whether to refetch the widget before resolving or not + * @param reexport Whether to re-export the widget before resolving or not * @return Promise of the widget */ - public Promise getWidget(JsVariableDefinition varDef, boolean refetch) { + public Promise getWidget(JsVariableDefinition varDef, boolean reexport) { return exportScopeTicket(varDef) .race(ticket -> { TypedTicket typedTicket = new TypedTicket(); typedTicket.setType(varDef.getType()); typedTicket.setTicket(ticket); - return getWidget(typedTicket, refetch); + return getExportedWidget(typedTicket, reexport); }).promise(); } @@ -1077,15 +1077,15 @@ public Promise getWidget(JsVariableDefinition varDef, boolean refetch) * Retrieve a widget using a ticket. Does not create a new ticket. * * @param typedTicket Ticket to fetch - * @param refetch Whether to refetch the widget. If false, you must ensure you call {@link JsWidget#refetch()} before + * @param reexport Whether to re-export the widget. If false, you must ensure you call {@link JsWidget#refetch()} before * using the widget. * @return Promise of the widget */ - public Promise getWidget(TypedTicket typedTicket, boolean refetch) { + private Promise getExportedWidget(TypedTicket typedTicket, boolean reexport) { return whenServerReady("get a widget") .then(response -> { JsWidget widget = new JsWidget(this, typedTicket); - if (refetch) { + if (reexport) { return widget.refetch(); } return Promise.resolve(widget); diff --git a/web/client-api/src/main/java/io/deephaven/web/client/api/widget/JsWidgetExportedObject.java b/web/client-api/src/main/java/io/deephaven/web/client/api/widget/JsWidgetExportedObject.java index 2a0858e112e..9e7090f301a 100644 --- a/web/client-api/src/main/java/io/deephaven/web/client/api/widget/JsWidgetExportedObject.java +++ b/web/client-api/src/main/java/io/deephaven/web/client/api/widget/JsWidgetExportedObject.java @@ -51,7 +51,7 @@ public boolean isOwner() { } public Promise fetch() { - return fetch(false); + return fetch(true); } private void verifyIsOwner() { @@ -72,7 +72,7 @@ public TypedTicket takeTicket() { /** * Fetches the object from the server. * - * @param takeOwnership Whether to take ownership of the object. Defaults to false. + * @param takeOwnership Whether to take ownership of the object. Defaults to true. * @return a promise that resolves to the object */ @JsMethod @@ -82,7 +82,7 @@ public Promise fetch(@JsOptional Boolean takeOwnership) { } verifyIsOwner(); if (takeOwnership) { - return this.connection.getObject(takeTicket()); + return this.connection.getExportedObject(takeTicket()); } return this.connection.getObject( new JsVariableDefinition(ticket.getType(), null, ticket.getTicket().getTicket_asB64(), null)); From 7b97ff5e1bdd7738a2bb80e667bd7c1237c01aed Mon Sep 17 00:00:00 2001 From: mikebender Date: Fri, 8 Dec 2023 13:08:57 -0500 Subject: [PATCH 19/20] Spotless --- .../java/io/deephaven/web/client/api/WorkerConnection.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/web/client-api/src/main/java/io/deephaven/web/client/api/WorkerConnection.java b/web/client-api/src/main/java/io/deephaven/web/client/api/WorkerConnection.java index 2ec3f76ba2a..aaa5e3f29cb 100644 --- a/web/client-api/src/main/java/io/deephaven/web/client/api/WorkerConnection.java +++ b/web/client-api/src/main/java/io/deephaven/web/client/api/WorkerConnection.java @@ -1077,8 +1077,8 @@ public Promise getWidget(JsVariableDefinition varDef, boolean reexport * Retrieve a widget using a ticket. Does not create a new ticket. * * @param typedTicket Ticket to fetch - * @param reexport Whether to re-export the widget. If false, you must ensure you call {@link JsWidget#refetch()} before - * using the widget. + * @param reexport Whether to re-export the widget. If false, you must ensure you call {@link JsWidget#refetch()} + * before using the widget. * @return Promise of the widget */ private Promise getExportedWidget(TypedTicket typedTicket, boolean reexport) { From 901ad90e67578db9c9248cbce929a8c3b20029fd Mon Sep 17 00:00:00 2001 From: mikebender Date: Fri, 8 Dec 2023 17:43:03 -0500 Subject: [PATCH 20/20] Fix up TreeTables - JsTreeTable was using the ticket from the widget directly, which doesn't work now if the widget releases it's ticket when the stream disconnects (fetch-only widgets) - Need to export the table as an exportedObject so we don't release it entirely - Will need Colin to review, this seems a little strange. --- .../HierarchicalTableTypePlugin.java | 1 + .../web/client/api/tree/JsTreeTable.java | 15 ++++++++++----- 2 files changed, 11 insertions(+), 5 deletions(-) diff --git a/plugin/hierarchicaltable/src/main/java/io/deephaven/hierarchicaltable/HierarchicalTableTypePlugin.java b/plugin/hierarchicaltable/src/main/java/io/deephaven/hierarchicaltable/HierarchicalTableTypePlugin.java index 381647d1b21..4b05c3afe3f 100644 --- a/plugin/hierarchicaltable/src/main/java/io/deephaven/hierarchicaltable/HierarchicalTableTypePlugin.java +++ b/plugin/hierarchicaltable/src/main/java/io/deephaven/hierarchicaltable/HierarchicalTableTypePlugin.java @@ -45,6 +45,7 @@ public void writeCompatibleObjectTo( .setSnapshotSchema(BarrageUtil.schemaBytes( fbb -> HierarchicalTableSchemaUtil.makeSchemaPayload(fbb, hierarchicalTable))) .build(); + exporter.reference(object); result.writeTo(out); } 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 4f7bd28067f..dd99e428223 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 @@ -364,6 +364,8 @@ private enum RebuildStep { // The current filter and sort state private List filters = new ArrayList<>(); private List sorts = new ArrayList<>(); + + private TypedTicket sourceTicket; private TicketAndPromise filteredTable; private TicketAndPromise sortedTable; @@ -396,6 +398,7 @@ private enum RebuildStep { public JsTreeTable(WorkerConnection workerConnection, JsWidget widget) { this.connection = workerConnection; this.widget = widget; + this.sourceTicket = widget.getExportedObjects()[0].takeTicket(); // register for same-session disconnect/reconnect callbacks this.connection.registerSimpleReconnectable(this); @@ -509,7 +512,7 @@ public JsTreeTable(WorkerConnection workerConnection, JsWidget widget) { .newState(this, (c, newState, metadata) -> { HierarchicalTableSourceExportRequest exportRequest = new HierarchicalTableSourceExportRequest(); exportRequest.setResultTableId(newState.getHandle().makeTicket()); - exportRequest.setHierarchicalTableId(widget.getTicket()); + exportRequest.setHierarchicalTableId(sourceTicket.getTicket()); connection.hierarchicalTableServiceClient().exportSource(exportRequest, connection.metadata(), c::apply); }, "source for hierarchical table") @@ -521,7 +524,7 @@ private TicketAndPromise prepareFilter() { return filteredTable; } if (nextFilters.isEmpty()) { - return new TicketAndPromise<>(widget.getTicket(), connection); + return new TicketAndPromise<>(sourceTicket.getTicket(), connection); } Ticket ticket = connection.getConfig().newTicket(); filteredTable = new TicketAndPromise<>(ticket, Callbacks.grpcUnaryPromise(c -> { @@ -529,7 +532,7 @@ private TicketAndPromise prepareFilter() { HierarchicalTableApplyRequest applyFilter = new HierarchicalTableApplyRequest(); applyFilter.setFiltersList( nextFilters.stream().map(FilterCondition::makeDescriptor).toArray(Condition[]::new)); - applyFilter.setInputHierarchicalTableId(widget.getTicket()); + applyFilter.setInputHierarchicalTableId(sourceTicket.getTicket()); applyFilter.setResultHierarchicalTableId(ticket); connection.hierarchicalTableServiceClient().apply(applyFilter, connection.metadata(), c::apply); }), connection); @@ -650,7 +653,7 @@ private void replaceSubscription(RebuildStep step) { this.alwaysFireNextEvent = false; JsLog.debug("Sending tree table request", this, - LazyString.of(() -> widget.getTicket().getTicket_asB64()), + LazyString.of(() -> sourceTicket.getTicket().getTicket_asB64()), columnsBitset, range, alwaysFireEvent); @@ -977,6 +980,8 @@ public void close() { JsLog.debug("Closing tree table", this); + connection.releaseTicket(sourceTicket.getTicket()); + connection.unregisterSimpleReconnectable(this); if (filteredTable != null) { @@ -1010,7 +1015,7 @@ public void close() { @Override public TypedTicket typedTicket() { - return widget.typedTicket(); + return sourceTicket; } /**