Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 20 commits into from

Commits on Dec 1, 2023

  1. 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)
    ```
    mofojed committed Dec 1, 2023
    Configuration menu
    Copy the full SHA
    f48b18f View commit details
    Browse the repository at this point in the history
  2. Configuration menu
    Copy the full SHA
    58aeab4 View commit details
    Browse the repository at this point in the history
  3. Fix up pandas table fetch

    - Now it doesn't need to do the extra round trip
    mofojed committed Dec 1, 2023
    Configuration menu
    Copy the full SHA
    6f77c64 View commit details
    Browse the repository at this point in the history
  4. WIP need to finish and clean it up

    - Have the exported object "own" the ticket until it is taken from it
    mofojed committed Dec 1, 2023
    Configuration menu
    Copy the full SHA
    75ce4fb View commit details
    Browse the repository at this point in the history

Commits on Dec 2, 2023

  1. 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 deephaven#4903
    mofojed committed Dec 2, 2023
    Configuration menu
    Copy the full SHA
    f8d8737 View commit details
    Browse the repository at this point in the history
  2. Configuration menu
    Copy the full SHA
    732938a View commit details
    Browse the repository at this point in the history

Commits on Dec 4, 2023

  1. Apply spotless

    mofojed committed Dec 4, 2023
    Configuration menu
    Copy the full SHA
    91e9adf View commit details
    Browse the repository at this point in the history
  2. Update web/client-api/src/main/java/io/deephaven/web/client/api/Worke…

    …rConnection.java
    
    Co-authored-by: Colin Alworth <colin@vertispan.com>
    mofojed and niloc132 authored Dec 4, 2023
    Configuration menu
    Copy the full SHA
    108936a View commit details
    Browse the repository at this point in the history
  3. Apply suggestions from code review

    Co-authored-by: Colin Alworth <colin@vertispan.com>
    mofojed and niloc132 authored Dec 4, 2023
    Configuration menu
    Copy the full SHA
    cbc0ae3 View commit details
    Browse the repository at this point in the history
  4. Clean up from code review

    mofojed committed Dec 4, 2023
    Configuration menu
    Copy the full SHA
    ee5ef03 View commit details
    Browse the repository at this point in the history
  5. Configuration menu
    Copy the full SHA
    8532cd1 View commit details
    Browse the repository at this point in the history

Commits on Dec 7, 2023

  1. Changes from code review

    mofojed committed Dec 7, 2023
    Configuration menu
    Copy the full SHA
    ba041a5 View commit details
    Browse the repository at this point in the history

Commits on Dec 8, 2023

  1. 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
    mofojed committed Dec 8, 2023
    Configuration menu
    Copy the full SHA
    59085d9 View commit details
    Browse the repository at this point in the history
  2. Configuration menu
    Copy the full SHA
    81ec321 View commit details
    Browse the repository at this point in the history
  3. Spotless

    mofojed committed Dec 8, 2023
    Configuration menu
    Copy the full SHA
    c9e36c9 View commit details
    Browse the repository at this point in the history
  4. Update web/client-api/src/main/java/io/deephaven/web/client/api/Worke…

    …rConnection.java
    
    Co-authored-by: Colin Alworth <colin@vertispan.com>
    mofojed and niloc132 authored Dec 8, 2023
    Configuration menu
    Copy the full SHA
    73172d3 View commit details
    Browse the repository at this point in the history
  5. Update web/client-api/src/main/java/io/deephaven/web/client/api/widge…

    …t/JsWidget.java
    
    Co-authored-by: Colin Alworth <colin@vertispan.com>
    mofojed and niloc132 authored Dec 8, 2023
    Configuration menu
    Copy the full SHA
    a1add7f View commit details
    Browse the repository at this point in the history
  6. 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
    mofojed committed Dec 8, 2023
    Configuration menu
    Copy the full SHA
    4b27d13 View commit details
    Browse the repository at this point in the history
  7. Spotless

    mofojed committed Dec 8, 2023
    Configuration menu
    Copy the full SHA
    7b97ff5 View commit details
    Browse the repository at this point in the history
  8. 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.
    mofojed committed Dec 8, 2023
    Configuration menu
    Copy the full SHA
    901ad90 View commit details
    Browse the repository at this point in the history