From f06b737e24b581aa5ebfc0c3580bf09a604f8e66 Mon Sep 17 00:00:00 2001 From: Colin Alworth Date: Mon, 11 Dec 2023 14:16:53 -0600 Subject: [PATCH 1/9] More docs, different approach to solve this --- .../web/client/api/widget/JsWidget.java | 60 +++++++++++-- .../api/widget/JsWidgetExportedObject.java | 90 ++++++++++++++----- 2 files changed, 120 insertions(+), 30 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 e5449ec42a9..d574ad2e80e 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 @@ -34,15 +34,57 @@ /** * A Widget represents a server side object that sends one or more responses to the client. The client can then * interpret these responses to see what to render, or how to respond. - * + *

* Most custom object types result in a single response being sent to the client, often with other exported objects, but * some will have streamed responses, and allow the client to send follow-up requests of its own. This class's API is * backwards compatible, but as such does not offer a way to tell the difference between a streaming or non-streaming * object type, the client code that handles the payloads is expected to know what to expect. See - * dh.WidgetMessageDetails for more information. - * + * {@link WidgetMessageDetails} for more information. + *

* When the promise that returns this object resolves, it will have the first response assigned to its fields. Later - * responses from the server will be emitted as "message" events. When the connection with the server ends + * responses from the server will be emitted as "message" events. When the connection with the server ends, the "close" + * event will be emitted. In this way, the connection will behave roughly in the same way as a WebSocket - either side + * can close, and after close no more messages will be processed. There can be some latency in closing locally while + * remote messages are still pending - it is up to implementations of plugins to handle this case. + *

+ * Also like WebSockets, the plugin API doesn't define how to serialize messages, and just handles any binary payloads. + * What it does handle however, is allowing those messages to include references to server-side objects with those + * payloads. Those server side objects might be tables or other built-in types in the Deephaven JS API, or could be + * objects usable through their own plugins. They also might have no plugin at all, allowing the client to hold a + * reference to them and pass them back to the server, either to the current plugin instance, or through another API. + * The {@code Widget} type does not specify how those objects should be used or their lifecycle, but leaves that + * entirely to the plugin. Messages will arrive in the order they were sent. + *

+ * This can suggest several patterns for how plugins operate: + *

+ * + * Handling server objects in messages also has more than one potential pattern that can be used: + * */ // TODO consider reconnect support? This is somewhat tricky without understanding the semantics of the widget @TsName(namespace = "dh", name = "Widget") @@ -120,9 +162,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(); @@ -226,8 +269,7 @@ default ArrayBufferView asView() { * @param references an array of objects that can be safely sent to the server */ @JsMethod - public void sendMessage(MessageUnion msg, - @JsOptional JsArray<@TsTypeRef(ServerObject.Union.class) ServerObject> references) { + public void sendMessage(MessageUnion msg, @JsOptional JsArray references) { if (messageStream == null) { return; } @@ -249,7 +291,7 @@ public void sendMessage(MessageUnion msg, } for (int i = 0; references != null && i < references.length; i++) { - ServerObject reference = references.getAt(i); + ServerObject reference = references.getAt(i).asServerObject(); data.addReferences(reference.typedTicket()); } 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..65afe6fab27 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,9 +6,12 @@ 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.session_pb.ExportRequest; import io.deephaven.javascript.proto.dhinternal.io.deephaven.proto.table_pb.ExportedTableCreationResponse; +import io.deephaven.javascript.proto.dhinternal.io.deephaven.proto.ticket_pb.Ticket; 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.JsLazy; import io.deephaven.web.client.api.JsTable; import io.deephaven.web.client.api.ServerObject; import io.deephaven.web.client.api.WorkerConnection; @@ -19,8 +22,7 @@ import jsinterop.annotations.JsProperty; /** - * Represents a server-side object that may not yet have been fetched by the client. Does not memoize its result, so - * fetch() should only be called once, and calling close() on this object will also close the result of the fetch. + * Represents a server-side object that may not yet have been fetched by the client. */ @TsInterface @TsName(namespace = "dh", name = "WidgetExportedObject") @@ -29,9 +31,32 @@ public class JsWidgetExportedObject implements ServerObject { private final TypedTicket ticket; + private final JsLazy> fetched; + public JsWidgetExportedObject(WorkerConnection connection, TypedTicket ticket) { this.connection = connection; this.ticket = ticket; + this.fetched = JsLazy.of(() -> { + if (getType() == null) { + return Promise.reject("Exported object has no type, can't be fetched"); + } + 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)); + } + }); } @JsProperty @@ -47,32 +72,55 @@ public TypedTicket typedTicket() { return typedTicket; } + /** + * Exports another copy of this reference, allowing it to be fetched separately. Results in rejection if the ticket + * was already closed (either by calling {@link #close()} or closing the object returned from {@link #fetch()}). + * @return a promise returning a reexported copy of this object, still referencing the same server-side object. + */ + public Promise reexport() { + Ticket reexportedTicket = connection.getConfig().newTicket(); + return Callbacks.grpcUnaryPromise(c -> { + ExportRequest req = new ExportRequest(); + req.setSourceId(ticket.getTicket()); + req.setResultId(reexportedTicket); + connection.sessionServiceClient().exportFromTicket(req, connection.metadata(), c::apply); + }).then(success -> { + TypedTicket typedTicket = new TypedTicket(); + typedTicket.setTicket(reexportedTicket); + typedTicket.setType(ticket.getType()); + return Promise.resolve(new JsWidgetExportedObject(connection, typedTicket)); + }); + } + + + // Could also return Promise - perhaps if we make it have a `boolean refetch` arg? + // /** + // * Returns a copy of this widget, so that any later {@link #fetch()} will always return a fresh instance. + // * @return + // */ + // public JsWidgetExportedObject copy() { + // return new JsWidgetExportedObject(connection, ticket); + // } + + /** + * Returns a promise that will fetch the object represented by this reference. Multiple calls to this will return + * the same instance. + * + * @return a promise that will resolve to a client side object that represents the reference on the server. + */ @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 fetched.get(); } /** - * Releases the server-side resources associated with this object, regardless of whether or not other client-side - * objects exist that also use that object. + * Releases the server-side resources associated with this object, regardless of whether other client-side objects + * exist that also use that object. Should not be called after fetch() has been invoked. */ @JsMethod public void close() { - connection.releaseTicket(ticket.getTicket()); + if (!fetched.isAvailable()) { + connection.releaseTicket(ticket.getTicket()); + } } } From f2007fc30076dcff5981636f4bdc5ee24ba2bdbd Mon Sep 17 00:00:00 2001 From: Colin Alworth Date: Mon, 11 Dec 2023 14:21:08 -0600 Subject: [PATCH 2/9] missing annotations --- .../deephaven/web/client/api/widget/JsWidgetExportedObject.java | 2 ++ 1 file changed, 2 insertions(+) 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 65afe6fab27..3c8d43b82ab 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 @@ -77,6 +77,7 @@ public TypedTicket typedTicket() { * was already closed (either by calling {@link #close()} or closing the object returned from {@link #fetch()}). * @return a promise returning a reexported copy of this object, still referencing the same server-side object. */ + @JsMethod public Promise reexport() { Ticket reexportedTicket = connection.getConfig().newTicket(); return Callbacks.grpcUnaryPromise(c -> { @@ -98,6 +99,7 @@ public Promise reexport() { // * Returns a copy of this widget, so that any later {@link #fetch()} will always return a fresh instance. // * @return // */ + // @JsMethod // public JsWidgetExportedObject copy() { // return new JsWidgetExportedObject(connection, ticket); // } From 657504b9815164b601576f333cfd97d3f8cddd87 Mon Sep 17 00:00:00 2001 From: Colin Alworth Date: Mon, 11 Dec 2023 14:18:26 -0600 Subject: [PATCH 3/9] Correctly release partitioned tables when closed from js --- .../java/io/deephaven/web/client/api/JsPartitionedTable.java | 2 ++ 1 file changed, 2 insertions(+) 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..628ee3d5180 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 @@ -314,6 +314,8 @@ public void close() { if (subscription != null) { subscription.close(); } + + widget.close(); } } From 6eacec9273f54f927ccdfb1a02fa930c0cdcd389 Mon Sep 17 00:00:00 2001 From: Colin Alworth Date: Mon, 11 Dec 2023 14:18:49 -0600 Subject: [PATCH 4/9] Correctly release hierarchical tables when closed from js --- .../main/java/io/deephaven/web/client/api/tree/JsTreeTable.java | 1 + 1 file changed, 1 insertion(+) 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..a503153b575 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 @@ -978,6 +978,7 @@ public void close() { JsLog.debug("Closing tree table", this); connection.unregisterSimpleReconnectable(this); + connection.releaseTicket(widget.getTicket()); // Presently it is never necessary to release widget tickets, since they can't be export tickets. // connection.releaseTicket(widget.getTicket()); From 95e99fd08632e4f25d4d291557c24ac9e9603446 Mon Sep 17 00:00:00 2001 From: Colin Alworth Date: Mon, 11 Dec 2023 20:54:49 -0600 Subject: [PATCH 5/9] Close hierarchical/pandas widgets, avoid double-fetch --- .../web/client/api/WorkerConnection.java | 19 ++++++++++--------- .../deephaven/web/client/ide/IdeSession.java | 4 ++-- 2 files changed, 12 insertions(+), 11 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 f9611a1bc57..2d9ab5b1def 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,11 @@ 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(JsWidget::refetch) + .then(widget -> { + widget.close(); + return widget.getExportedObjects()[0].fetch(); + }); } else if (JsVariableType.PARTITIONEDTABLE.equalsIgnoreCase(definition.getType())) { return getPartitionedTable(definition); } else if (JsVariableType.HIERARCHICALTABLE.equalsIgnoreCase(definition.getType())) { @@ -780,7 +784,7 @@ public Promise getObject(JsVariableDefinition definition) { JsLog.warn( "TreeTable is now HierarchicalTable, fetching as a plain widget. To fetch as a HierarchicalTable use that as this type."); } - return getWidget(definition); + return getWidget(definition).then(JsWidget::refetch); } } @@ -911,12 +915,8 @@ public Promise getPartitionedTable(JsVariableDefinition varD .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 getHierarchicalTable(JsVariableDefinition varDef) { - return getWidget(varDef).then(w -> Promise.resolve(new JsTreeTable(this, w))); + return getWidget(varDef).then(JsWidget::refetch).then(w -> Promise.resolve(new JsTreeTable(this, w))); } public Promise getFigure(JsVariableDefinition varDef) { @@ -926,13 +926,14 @@ public Promise getFigure(JsVariableDefinition varDef) { return whenServerReady("get a figure") .then(server -> new JsFigure(this, c -> { - getWidget(varDef).then(widget -> { + getWidget(varDef).then(JsWidget::refetch).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); @@ -960,7 +961,7 @@ public Promise getWidget(JsVariableDefinition varDef) { public Promise getWidget(TypedTicket typedTicket) { return whenServerReady("get a widget") - .then(response -> new JsWidget(this, typedTicket).refetch()); + .then(response -> Promise.resolve(new JsWidget(this, typedTicket))); } public void registerSimpleReconnectable(HasLifecycle figure) { 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 5fabcc7af5e2503e91a8b431ba5a28e934e6a7d6 Mon Sep 17 00:00:00 2001 From: Colin Alworth Date: Tue, 12 Dec 2023 09:31:20 -0600 Subject: [PATCH 6/9] Avoid re-exporting tickets from exports --- .../web/client/api/WorkerConnection.java | 64 +++++++++++++++---- .../api/console/JsVariableDefinition.java | 4 ++ .../api/widget/JsWidgetExportedObject.java | 4 +- 3 files changed, 56 insertions(+), 16 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 2d9ab5b1def..7d4ea477962 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 @@ -776,14 +776,7 @@ public Promise getObject(JsVariableDefinition definition) { } else if (JsVariableType.HIERARCHICALTABLE.equalsIgnoreCase(definition.getType())) { return getHierarchicalTable(definition); } else { - if (JsVariableType.TABLEMAP.equalsIgnoreCase(definition.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(definition.getType())) { - JsLog.warn( - "TreeTable is now HierarchicalTable, fetching as a plain widget. To fetch as a HierarchicalTable use that as this type."); - } + warnLegacyTicketTypes(definition.getType()); return getWidget(definition).then(JsWidget::refetch); } } @@ -814,6 +807,45 @@ public Promise getJsObject(JsPropertyMap definitionObject) { } } + public Promise getObject(TypedTicket typedTicket) { + if (JsVariableType.TABLE.equalsIgnoreCase(typedTicket.getType())) { + throw new IllegalArgumentException("wrong way to get a table from a ticket"); + } else if (JsVariableType.FIGURE.equalsIgnoreCase(typedTicket.getType())) { + return new JsFigure(this, c -> { + JsWidget widget = new JsWidget(this, typedTicket); + widget.refetch().then(ignore -> { + c.apply(null, makeFigureFetchResponse(widget)); + return null; + }); + }).refetch(); + } else if (JsVariableType.PANDAS.equalsIgnoreCase(typedTicket.getType())) { + return getWidget(typedTicket) + .then(JsWidget::refetch) + .then(widget -> { + widget.close(); + return widget.getExportedObjects()[0].fetch(); + }); + } else if (JsVariableType.PARTITIONEDTABLE.equalsIgnoreCase(typedTicket.getType())) { + return new JsPartitionedTable(this, new JsWidget(this, typedTicket)).refetch(); + } else if (JsVariableType.HIERARCHICALTABLE.equalsIgnoreCase(typedTicket.getType())) { + return new JsWidget(this, typedTicket).refetch().then(w -> Promise.resolve(new JsTreeTable(this, w))); + } else { + warnLegacyTicketTypes(typedTicket.getType()); + return getWidget(typedTicket).then(JsWidget::refetch); + } + } + + private static void warnLegacyTicketTypes(String ticketType) { + if (JsVariableType.TABLEMAP.equalsIgnoreCase(ticketType)) { + 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(ticketType)) { + JsLog.warn( + "TreeTable is now HierarchicalTable, fetching as a plain widget. To fetch as a HierarchicalTable use that as this type."); + } + } + @JsMethod @SuppressWarnings("ConstantConditions") public JsRunnable subscribeToFieldUpdates(JsConsumer callback) { @@ -927,12 +959,7 @@ public Promise getFigure(JsVariableDefinition varDef) { .then(server -> new JsFigure(this, c -> { getWidget(varDef).then(JsWidget::refetch).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); + c.apply(null, makeFigureFetchResponse(widget)); widget.close(); return null; }, error -> { @@ -942,6 +969,15 @@ public Promise getFigure(JsVariableDefinition varDef) { }).refetch()); } + private static FetchObjectResponse makeFigureFetchResponse(JsWidget 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)); + return legacyResponse; + } + private TypedTicket createTypedTicket(JsVariableDefinition varDef) { TypedTicket typedTicket = new TypedTicket(); typedTicket.setTicket(TableTicket.createTicket(varDef)); diff --git a/web/client-api/src/main/java/io/deephaven/web/client/api/console/JsVariableDefinition.java b/web/client-api/src/main/java/io/deephaven/web/client/api/console/JsVariableDefinition.java index 11473ea63be..576a0cb74c5 100644 --- a/web/client-api/src/main/java/io/deephaven/web/client/api/console/JsVariableDefinition.java +++ b/web/client-api/src/main/java/io/deephaven/web/client/api/console/JsVariableDefinition.java @@ -28,6 +28,10 @@ public class JsVariableDefinition { private final String applicationName; public JsVariableDefinition(String type, String title, String id, String description) { + // base64('s/') ==> 'cy8' + if (!id.startsWith("cy8")) { + throw new IllegalArgumentException("Cannot create a VariableDefinition from a non-scope ticket"); + } this.type = type; this.title = title == null ? JS_UNAVAILABLE : title; this.id = id; 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 3c8d43b82ab..c4af4c77c92 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 @@ -53,8 +53,7 @@ public JsWidgetExportedObject(WorkerConnection connection, TypedTicket ticket) { return Promise.resolve(table); }); } else { - return this.connection.getObject( - new JsVariableDefinition(ticket.getType(), null, ticket.getTicket().getTicket_asB64(), null)); + return this.connection.getObject(ticket); } }); } @@ -75,6 +74,7 @@ public TypedTicket typedTicket() { /** * Exports another copy of this reference, allowing it to be fetched separately. Results in rejection if the ticket * was already closed (either by calling {@link #close()} or closing the object returned from {@link #fetch()}). + * * @return a promise returning a reexported copy of this object, still referencing the same server-side object. */ @JsMethod From 64fbabbbebe4fead01d16c0130c2111203d4ca06 Mon Sep 17 00:00:00 2001 From: Colin Alworth Date: Tue, 12 Dec 2023 10:02:44 -0600 Subject: [PATCH 7/9] pre-review cleanup --- .../api/console/JsVariableDefinition.java | 4 +-- .../web/client/api/tree/JsTreeTable.java | 4 +-- .../web/client/api/widget/JsWidget.java | 4 +++ .../api/widget/JsWidgetExportedObject.java | 28 +++++++++++-------- 4 files changed, 23 insertions(+), 17 deletions(-) diff --git a/web/client-api/src/main/java/io/deephaven/web/client/api/console/JsVariableDefinition.java b/web/client-api/src/main/java/io/deephaven/web/client/api/console/JsVariableDefinition.java index 576a0cb74c5..50dda49996d 100644 --- a/web/client-api/src/main/java/io/deephaven/web/client/api/console/JsVariableDefinition.java +++ b/web/client-api/src/main/java/io/deephaven/web/client/api/console/JsVariableDefinition.java @@ -12,8 +12,8 @@ /** * A format to describe a variable available to be read from the server. Application fields are optional, and only * populated when a variable is provided by application mode. - * - * APIs which take a VariableDefinition` must at least be provided an object with a type and id field. + *

+ * APIs which take a VariableDefinition must at least be provided an object with a type and id field. */ @TsInterface @TsName(namespace = "dh.ide", name = "VariableDefinition") 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 a503153b575..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 @@ -978,10 +978,8 @@ public void close() { JsLog.debug("Closing tree table", this); connection.unregisterSimpleReconnectable(this); - connection.releaseTicket(widget.getTicket()); - // 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/JsWidget.java b/web/client-api/src/main/java/io/deephaven/web/client/api/widget/JsWidget.java index d574ad2e80e..94e3ad7edfb 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 @@ -218,6 +218,10 @@ public String getDataAsString() { return new String(Js.uncheckedCast(response.getData().getPayload_asU8()), StandardCharsets.UTF_8); } + /** + * @return the exported objects sent in the initial message from the server. The client is responsible for closing + * them when finished using them. + */ @Override @JsProperty public JsWidgetExportedObject[] getExportedObjects() { 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 c4af4c77c92..263944a65e4 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 @@ -5,6 +5,7 @@ import com.vertispan.tsdefs.annotations.TsInterface; import com.vertispan.tsdefs.annotations.TsName; +import elemental2.core.JsArray; import elemental2.promise.Promise; import io.deephaven.javascript.proto.dhinternal.io.deephaven.proto.session_pb.ExportRequest; import io.deephaven.javascript.proto.dhinternal.io.deephaven.proto.table_pb.ExportedTableCreationResponse; @@ -19,10 +20,13 @@ import io.deephaven.web.client.api.console.JsVariableType; import io.deephaven.web.client.state.ClientTableState; import jsinterop.annotations.JsMethod; +import jsinterop.annotations.JsNullable; import jsinterop.annotations.JsProperty; /** - * Represents a server-side object that may not yet have been fetched by the client. + * Represents a server-side object that may not yet have been fetched by the client. When this object will no longer be + * used, if {@link #fetch()} is not called on this object, then {@link #close()} must be to ensure server-side resources + * are correctly freed. */ @TsInterface @TsName(namespace = "dh", name = "WidgetExportedObject") @@ -58,8 +62,19 @@ public JsWidgetExportedObject(WorkerConnection connection, TypedTicket ticket) { }); } + /** + * Returns the type of this export, typically one of {@link JsVariableType}, but may also include plugin types. If + * null, this object cannot be fetched, but can be passed to the server, such as via + * {@link JsWidget#sendMessage(JsWidget.MessageUnion, JsArray)}. + * + * @return the string type of this server-side object, or null. + */ + @JsNullable @JsProperty public String getType() { + if (ticket.getType().isEmpty()) { + return null; + } return ticket.getType(); } @@ -93,17 +108,6 @@ public Promise reexport() { }); } - - // Could also return Promise - perhaps if we make it have a `boolean refetch` arg? - // /** - // * Returns a copy of this widget, so that any later {@link #fetch()} will always return a fresh instance. - // * @return - // */ - // @JsMethod - // public JsWidgetExportedObject copy() { - // return new JsWidgetExportedObject(connection, ticket); - // } - /** * Returns a promise that will fetch the object represented by this reference. Multiple calls to this will return * the same instance. From f4fe85bf496ce6aacc7a963a6f90cd128b70d278 Mon Sep 17 00:00:00 2001 From: Colin Alworth Date: Tue, 12 Dec 2023 11:50:48 -0600 Subject: [PATCH 8/9] Cleanup unused imports --- .../main/java/io/deephaven/web/client/api/widget/JsWidget.java | 1 - .../deephaven/web/client/api/widget/JsWidgetExportedObject.java | 1 - 2 files changed, 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 94e3ad7edfb..058df82cc9b 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 @@ -4,7 +4,6 @@ package io.deephaven.web.client.api.widget; import com.vertispan.tsdefs.annotations.TsName; -import com.vertispan.tsdefs.annotations.TsTypeRef; import com.vertispan.tsdefs.annotations.TsUnion; import com.vertispan.tsdefs.annotations.TsUnionMember; import elemental2.core.ArrayBuffer; 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 263944a65e4..037f3d589dc 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 @@ -16,7 +16,6 @@ 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; From caddde67f6b84e5533d49bffa6d33afd229ab9d4 Mon Sep 17 00:00:00 2001 From: Colin Alworth Date: Tue, 12 Dec 2023 14:43:37 -0600 Subject: [PATCH 9/9] Review feedback --- .../io/deephaven/web/client/api/widget/JsWidget.java | 2 +- .../web/client/api/widget/JsWidgetExportedObject.java | 11 ++++++++++- 2 files changed, 11 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 058df82cc9b..22f2d347551 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 @@ -74,7 +74,7 @@ * * Handling server objects in messages also has more than one potential pattern that can be used: *

    - *
  • One object per message - the message clearly is about that message, no other details required.
    • + *
  • One object per message - the message clearly is about that object, no other details required.
    • *
  • Objects indexed within their message - as each message comes with a list of objects, those objects can be * referenced within the payload by index. This is roughly how {@link io.deephaven.web.client.api.widget.plot.JsFigure} * behaves, where the figure descriptor schema includes an index for each created series, describing which table should 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 037f3d589dc..d6df425e1e9 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 @@ -17,6 +17,7 @@ import io.deephaven.web.client.api.ServerObject; import io.deephaven.web.client.api.WorkerConnection; import io.deephaven.web.client.api.console.JsVariableType; +import io.deephaven.web.client.fu.JsLog; import io.deephaven.web.client.state.ClientTableState; import jsinterop.annotations.JsMethod; import jsinterop.annotations.JsNullable; @@ -94,6 +95,9 @@ public TypedTicket typedTicket() { @JsMethod public Promise reexport() { Ticket reexportedTicket = connection.getConfig().newTicket(); + + // Future optimization - we could "race" these by running the export in the background, to avoid + // an extra round trip. return Callbacks.grpcUnaryPromise(c -> { ExportRequest req = new ExportRequest(); req.setSourceId(ticket.getTicket()); @@ -115,7 +119,10 @@ public Promise reexport() { */ @JsMethod public Promise fetch() { - return fetched.get(); + if (getType() != null) { + return fetched.get(); + } + return Promise.reject("Can't fetch an object with no type (i.e. no server plugin implementation)"); } /** @@ -126,6 +133,8 @@ public Promise fetch() { public void close() { if (!fetched.isAvailable()) { connection.releaseTicket(ticket.getTicket()); + } else { + JsLog.warn("Cannot close, already fetched. Instead, close the fetched object."); } } }