Fix JS client leaking exported references, clarify dh.Widget API

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();
     }
 
 }

web/client-api/src/main/java/io/deephaven/web/client/api/WorkerConnection.java

@@ -766,21 +766,18 @@ 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())) {
             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.");
-            }
-            return getWidget(definition);
+            warnLegacyTicketTypes(definition.getType());
+            return getWidget(definition).then(JsWidget::refetch);
         }
     }
 
@@ -810,6 +807,45 @@ public Promise<?> getJsObject(JsPropertyMap<Object> 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<JsVariableChanges> callback) {
@@ -911,12 +947,8 @@ public Promise<JsPartitionedTable> getPartitionedTable(JsVariableDefinition varD
                 .then(widget -> new JsPartitionedTable(this, widget).refetch());
     }
 
-    public Promise<JsTreeTable> getTreeTable(JsVariableDefinition varDef) {
-        return getWidget(varDef).then(w -> Promise.resolve(new JsTreeTable(this, w)));
-    }
-
     public Promise<JsTreeTable> 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<JsFigure> getFigure(JsVariableDefinition varDef) {
@@ -926,13 +958,9 @@ public Promise<JsFigure> 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::typedTicket).toArray(TypedTicket[]::new));
-                                c.apply(null, legacyResponse);
+                            getWidget(varDef).then(JsWidget::refetch).then(widget -> {
+                                c.apply(null, makeFigureFetchResponse(widget));
+                                widget.close();
                                 return null;
                             }, error -> {
                                 c.apply(error, null);
@@ -941,6 +969,15 @@ public Promise<JsFigure> 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));
@@ -960,7 +997,7 @@ public Promise<JsWidget> getWidget(JsVariableDefinition varDef) {
 
     public Promise<JsWidget> 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) {

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 <b>type</b> and <b>id</b> field.
+ * <p>
+ * APIs which take a VariableDefinition must at least be provided an object with a <b>type</b> and <b>id</b> field.
  */
 @TsInterface
 @TsName(namespace = "dh.ide", name = "VariableDefinition")
@@ -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;

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();

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;
@@ -34,15 +33,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.
- *
+ * <p>
  * 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.
+ * <p>
  * 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.
+ * <p>
+ * 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.
+ * <p>
+ * This can suggest several patterns for how plugins operate:
+ * <ul>
+ * <li>The plugin merely exists to transport some other object to the client. This can be useful for objects which can
+ * easily be translated to some other type (like a Table) when the user clicks on it. An example of this is
+ * {@code pandas.DataFrame} will result in a widget that only contains a static
+ * {@link io.deephaven.web.client.api.JsTable}. Presently, the widget is immediately closed, and only the Table is
+ * provided to the JS API consumer.</li>
+ * <li>The plugin provides references to Tables and other objects, and those objects can live longer than the object
+ * which provided them. One concrete example of this could have been
+ * {@link io.deephaven.web.client.api.JsPartitionedTable} when fetching constituent tables, but it was implemented
+ * before bidirectional plugins were implemented. Another example of this is plugins that serve as a "factory", giving
+ * the user access to table manipulation/creation methods not supported by gRPC or the JS API.</li>
+ * <li>The plugin provides reference to Tables and other objects that only make sense within the context of the widget
+ * instance, so when the widget goes away, those objects should be released as well. This is also an example of
+ * {@link io.deephaven.web.client.api.JsPartitionedTable}, as the partitioned table tracks creation of new keys through
+ * an internal table instance.</li>
+ * </ul>
+ *
+ * Handling server objects in messages also has more than one potential pattern that can be used:
+ * <ul>
+ * <li>One object per message - the message clearly is about that object, no other details required.</li>
+ * <li>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
+ * be used, which columns should be mapped to each axis.</li>
+ * <li>Objects indexed since widget creation - each message would append its objects to a list created when the widget
+ * was first made, and any new exports that arrive in a new message would be appended to that list. Then, subsequent
+ * messages can reference objects already sent. This imposes a limitation where the client cannot release any exports
+ * without the server somehow signaling that it will never reference that export again.</li>
+ * </ul>
  */
 // TODO consider reconnect support? This is somewhat tricky without understanding the semantics of the widget
 @TsName(namespace = "dh", name = "Widget")
@@ -120,9 +161,10 @@ public Promise<JsWidget> 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();
@@ -175,6 +217,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() {
@@ -226,8 +272,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<ServerObject.Union> references) {
         if (messageStream == null) {
             return;
         }
@@ -249,7 +294,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());
         }