Fix JS client leaking exported references, clarify dh.Widget API (#4939)

JsPartitionedTable, JsFigure, PandasTable were previously being leaked
even with correct usage, ensure that those are released when client
code calls close(). Additionally, partitioned tables would be
re-exported unnecessarily, but only the re-export was released,
leaking the original reference.

Widget and WidgetExportedObject had documentation indicating how to
consume exported objects, but these rules were not enforced. The rules
have changed to be more consistent and are enforced, which should
avoid later confusion, and new documentation added to propose
potential patterns to follow when authoring plugins.

Fixes #4903

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