fix: Clean up how objects are managed when they are fetched #4909

mofojed · 2023-12-02T05:37:46Z

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 Widgets cause leaks, do not release all resources #4903

- 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) ```

- Now it doesn't need to do the extra round trip

- Have the exported object "own" the ticket until it is taken from it

- 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 deephaven#4903

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

web/client-api/src/main/java/io/deephaven/web/client/api/tree/JsTreeTable.java

web/client-api/src/main/java/io/deephaven/web/client/api/widget/JsWidgetExportedObject.java

niloc132 · 2023-12-04T17:47:49Z

web/client-api/src/main/java/io/deephaven/web/client/api/widget/JsWidgetExportedObject.java

+            takeOwnership = false;
+        }
+        if (takeOwnership) {
+            return this.connection.getObject(takeTicket());


this means that raced calls to fetch(true) and fetch(false) are going to intermittently fail - probably need some clearer docs on this and other general usage of ownership.

Was debating just having a separate method take instead of making it a boolean.
Really you should only be calling one or the other (true or false) depending on how you're managing the object, I wouldn't expect you'd want to call both.
In any case, I can add a verifyIsOwner() here in the false case, and throw if it's been called after a fetch(false) so it reduces a race that way, but you could still race fetch(false) then fetch(true) I suppose.

niloc132 · 2023-12-04T17:50:32Z

web/client-api/src/main/java/io/deephaven/web/client/api/widget/JsWidgetExportedObject.java

        }
+        return this.connection.getObject(
+                new JsVariableDefinition(ticket.getType(), null, ticket.getTicket().getTicket_asB64(), null));


I'm surprised this works - VariableDefinition is meant for scope tickets...

Will think on it more, but I think I'd prefer we decompose this (or provide an interface and second impl) to let this be reexported-and-fetched.

Yea, that was already there. Looks like I added this back when implementing JsWidget: #1888 (comment)
I may have misinterpreted that comment then.
Should be able to refactor the WorkerConnection#getObject methods; the only part of the JsVariableDefinition that is actually used is the type and the id used in exportScopeTicket

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

…rConnection.java Co-authored-by: Colin Alworth <colin@vertispan.com>

Co-authored-by: Colin Alworth <colin@vertispan.com>

- Can release the ticket of a widget automatically if the server closes the object - no way to reconnect (currently) - Remove unnecessary closes

web/client-api/src/main/java/io/deephaven/web/client/api/widget/JsWidget.java

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

niloc132 · 2023-12-08T17:45:10Z

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

+     * @param refetch Whether to refetch the widget before resolving or not
+     * @return Promise of the widget
+     */
+    public Promise<JsWidget> getWidget(JsVariableDefinition varDef, boolean refetch) {


instead of "refetch" could we use something like "export" or "re-export"? Most of the time "Fetch" means to get the object from the server (usually by name), but this isn't doing that again

The refetch in this case is triggering widget.refetch(), which was already named... but essentially is restarting the stream and fetching the data. I think the reexport name probably doesn't fit here either, and refetch makes more sense (or JsWidget.refetch should be renamed?). Thoughts?

…rConnection.java Co-authored-by: Colin Alworth <colin@vertispan.com>

…t/JsWidget.java Co-authored-by: Colin Alworth <colin@vertispan.com>

- 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

mofojed · 2023-12-08T21:31:59Z

Rollups are screwed up, need to fix...

- 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.

mofojed · 2023-12-08T22:49:54Z

@niloc132 So the issue now with releasing the widget ticket when the stream ends (as suggested in our previous discussion for fetch-only widgets) actually breaks JsTreeTable implementation, as it's using the ticket right from the widget for the hierarchical table??

deephaven-core/web/client-api/src/main/java/io/deephaven/web/client/api/tree/JsTreeTable.java

Line 512 in 1607606

exportRequest.setHierarchicalTableId(widget.getTicket());

So I changed HierarchicalTableTypePlugin to export a reference to the HierarchicalTable as an exported object... but I think JsPartitionedTable still has a problem... they're not expecting the widget ticket to be released after the widget has been fetched, which is at odds with what we're doing with Pandas (I'm actually not sure where the Pandas plugin is/how that works?).

I favour what I had previously, where the consumer needs to explicitly close the widget when they're done with it to release the ticket, rather than implicitly releasing the ticket when the stream ends... thoughts?

mofojed · 2023-12-12T19:21:05Z

Closed in favour of #4939

mofojed added 6 commits December 1, 2023 11:07

Release the widget after getting the figure

58aeab4

Fix up pandas table fetch

6f77c64

- Now it doesn't need to do the extra round trip

WIP need to finish and clean it up

75ce4fb

- Have the exported object "own" the ticket until it is taken from it

Clean up a bit more based on self review

732938a

mofojed added NoDocumentationNeeded NoReleaseNotesNeeded No release notes are needed. labels Dec 2, 2023

mofojed requested a review from niloc132 December 2, 2023 05:37

mofojed self-assigned this Dec 2, 2023

Apply spotless

91e9adf

mofojed mentioned this pull request Dec 4, 2023

feat(ui): Only send new exported objects deephaven/deephaven-plugins#129

Merged

niloc132 reviewed Dec 4, 2023

View reviewed changes

mofojed and others added 4 commits December 4, 2023 14:02

Update web/client-api/src/main/java/io/deephaven/web/client/api/Worke…

108936a

…rConnection.java Co-authored-by: Colin Alworth <colin@vertispan.com>

Apply suggestions from code review

cbc0ae3

Co-authored-by: Colin Alworth <colin@vertispan.com>

Clean up from code review

ee5ef03

Always verify the ownership when fetching exported object

8532cd1

mofojed requested a review from niloc132 December 5, 2023 21:14

niloc132 added jsapi plug-ins labels Dec 7, 2023

mofojed added 4 commits December 7, 2023 17:32

Changes from code review

ba041a5

Cleanup after review

59085d9

- Can release the ticket of a widget automatically if the server closes the object - no way to reconnect (currently) - Remove unnecessary closes

Remove unnecessary ticket release

81ec321

Spotless

c9e36c9

niloc132 reviewed Dec 8, 2023

View reviewed changes

mofojed and others added 3 commits December 8, 2023 12:47

Update web/client-api/src/main/java/io/deephaven/web/client/api/Worke…

73172d3

…rConnection.java Co-authored-by: Colin Alworth <colin@vertispan.com>

Update web/client-api/src/main/java/io/deephaven/web/client/api/widge…

a1add7f

…t/JsWidget.java Co-authored-by: Colin Alworth <colin@vertispan.com>

Clean up based on review

4b27d13

- 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

mofojed requested a review from niloc132 December 8, 2023 18:07

Spotless

7b97ff5

niloc132 mentioned this pull request Dec 12, 2023

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

Merged

mofojed closed this Dec 12, 2023

github-actions bot locked and limited conversation to collaborators Dec 12, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Clean up how objects are managed when they are fetched #4909

fix: Clean up how objects are managed when they are fetched #4909

mofojed commented Dec 2, 2023

niloc132 Dec 4, 2023

mofojed Dec 4, 2023

niloc132 Dec 4, 2023

mofojed Dec 4, 2023

niloc132 Dec 8, 2023

mofojed Dec 8, 2023

mofojed commented Dec 8, 2023

mofojed commented Dec 8, 2023

mofojed commented Dec 12, 2023

fix: Clean up how objects are managed when they are fetched #4909

fix: Clean up how objects are managed when they are fetched #4909

Conversation

mofojed commented Dec 2, 2023

niloc132 Dec 4, 2023

Choose a reason for hiding this comment

mofojed Dec 4, 2023

Choose a reason for hiding this comment

niloc132 Dec 4, 2023

Choose a reason for hiding this comment

mofojed Dec 4, 2023

Choose a reason for hiding this comment

niloc132 Dec 8, 2023

Choose a reason for hiding this comment

mofojed Dec 8, 2023

Choose a reason for hiding this comment

mofojed commented Dec 8, 2023

mofojed commented Dec 8, 2023

mofojed commented Dec 12, 2023