-
Notifications
You must be signed in to change notification settings - Fork 16
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: Callable cleanup race (DH-18536) #1113
fix: Callable cleanup race (DH-18536) #1113
Conversation
mofojed
commented
Feb 10, 2025
- Client was calling a closeCallable function each time any callable was no longer rendered
- Callables as part of the regular render cycle that didn't change were being recreated every time
- Since it wasn't the same object between renders, closeCallable was getting called for a previous instance of that callable but with the same ID
- Server side was unnecessarily listening for those to close callables as part of the regular render cycle
- Things that are no longer rendered are already cleaned up by the server, don't need to/shouldn't listen to the client for when to clean those up. Only needed for the temporary callables
- Tested by using the snippet from DH-18536:
- Also tested using the snippets from previous callable implentation ticket: feat: Return callables from callables in Deephaven UI #540
- Client was calling a closeCallable function each time any callable was no longer rendered - Callables as part of the regular render cycle that didn't change were being recreated every time - Since it wasn't the same object between renders, closeCallable was getting called for a previous instance of that callable but with the same ID - Server side was unnecessarily listening for those to close callables as part of the regular render cycle - Things that are no longer rendered are already cleaned up by the server, don't need to/shouldn't listen to the client for when to clean those up. Only needed for the temporary callables - Tested by using the snippet from DH-18536: ```python from deephaven import ui, time_table @ui.component def ui_resetable_table(): table, set_table = ui.use_state(lambda: time_table("PT1s")) handle_press = ui.use_liveness_scope(lambda _: set_table(time_table("PT1s")), []) return [ ui.action_button( "Reset", on_press=handle_press, ), table, ] resetable_table = ui_resetable_table() ``` - Also tested using the snippets from previous callable implentation ticket: deephaven#540 ```python from deephaven import ui @ui.component def my_comp(): on_press = ui.use_callback(lambda d: print(d), []) on_press_nested = ui.use_callback(lambda: print, []) on_press_double_nested = ui.use_callback(lambda: { "nestedFn": print, "some_val": 4 }, []) on_press_unserializable = ui.use_callback(lambda: set([1, 2, 3]), []) return [ ui.button("Normal", on_press=on_press), ui.button("Nested", on_press=on_press_nested), ui.button("Double Nested", on_press=on_press_double_nested), ui.button("Not Serializable", on_press=on_press_unserializable) ] c = my_comp() ```
ui docs preview (Available for 14 days) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. One thought, but it doesn't matter to me if it's implemented since the current logic is probably simpler to understand
// Keep track of callables that are currently rendered/in use | ||
// We want to retain the same callable between renders, so we use a map that persists between renders | ||
const renderedCallableMap = useRef( | ||
new Map<string, (...args: unknown[]) => void>() | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if you could put a map in wrapCallable
(or pass one in) and then stick the callable in a WeakRef
so the map doesn't retain it forever.
Or you could do that here and reduce the logic a bit (like no need for the cleanup step or dead map I think). Not a big deal though. Something like this maybe?
I think there's a potential downside here of the map containing dead callable entries still. They would be valued to GCed weak refs, so you could clean up the map by iterating it and filtering out weakrefs that have been cleaned. Or use another finalization registry to clean the map. Might end up at a similar amount of logic at this point
const callableMap = useRef(new Map<string, WeakRef<(...args: unknown[]) => void>>());
if (callableMap.current.has(callableId)) {
const callable = callableMap.current.get(callableId)?.deref();
if (callable) {
return callable;
}
}
const callable = wrapCallable(jsonClient, callableId, callableFinalizationRegistry);
callableMap.current.set(callableId, new WeakRef(callable));
return callable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure when that case would occur? Having it in this map means it's currently used in the rendered document. Since it's already referenced by the rendered document, using a weak ref here would be pointless.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The downside in my comment was about using a weakref. It was mostly just a thought of a way to keep the close handles consistent (or have the wrapCallable
util handle the lifecycle still), but I don't think it will matter and would just complicate things a bit since we have a single point everything in render goes through anyway.
You could add a registerFinalizer
or something to wrapCallable
so you don't have to remove them from the finalization registry after. Since right now the callable gets added and then removed from finzliation, but finalization is never called (for these render cycle callables)
Do you want to put the ticket number in the title so it's easier to update Jira when the plugin version is updated in enterprise? |
- Pass a shouldRegister function to wrapCallable, so we don't bother registering with the registry if we don't want to
a3691b6
ui docs preview (Available for 14 days) |