Remove all observers on YChat.unobserve()#463
Conversation
YChat.unobserve()
|
@brichet This PR fixes a memory leak by making Typically So even though this is technically a breaking change (calling Ideally, there would be some kind of |
Extend the room GC coverage so leaks in the Jupyter YDoc shared models themselves (not just consumer-added observers) are caught -- e.g. YChat previously never removed its __init__ observers, leaking the room. - test_yroom_gc.py: add TestYRoomGC (the YRoom is collected after stop) and TestYRoomSharedModelGC (the YBaseDoc model is collected), each parametrized over file/notebook/chat x graceful/immediate. The chat cases skip when jupyterlab_chat is not installed. - conftest: make_room_file / make_yroom take a (file|notebook|chat), build the right file + room_id, and create rooms via the manager factory (so YNotebookRoom is used for notebooks). Track rooms weakly so the fixture never pins a room. Give MockServerDocsApp an outputs_manager=None attribute so notebook rooms load with the outputs service disabled (the default). - unit-tests CI: install jupyterlab-chat so the YChat GC tests run. The YChat cases require the observer-cleanup fix in jupyter-chat >=0.23.0a2 (jupyterlab/jupyter-chat#463); until that releases these tests fail in CI, surfacing the leak.
brichet
left a comment
There was a problem hiding this comment.
Thanks @dlqqq for working on this.
Perhaps this is expected at that stage, but the new tests fail with jupyter-collaboration >= 5.
jupyter-collaboration==4.4.1
$ pip freeze | grep jupyter-collaboration
jupyter-collaboration==4.4.1
jupyter-collaboration-ui==2.4.1
$ pytest
============================= test session starts ==============================
platform linux -- Python 3.14.6, pytest-9.1.1, pluggy-1.6.0
rootdir: /home/brichet/projects/jupyter-chat
configfile: pyproject.toml
plugins: cov-7.1.0, anyio-4.14.1, asyncio-1.4.0
asyncio: mode=Mode.STRICT, debug=False, asyncio_default_fixture_loop_scope=None, asyncio_default_test_loop_scope=function
collected 22 items
python/jupyterlab-chat/jupyterlab_chat/tests/test_imports.py . [ 4%]
python/jupyterlab-chat/jupyterlab_chat/tests/test_ychat.py ............. [ 63%]
..... [ 86%]
python/jupyterlab-chat/jupyterlab_chat/tests/test_ychat_gc.py ... [100%]
============================== 22 passed in 1.01s ==============================jupyter-collaboration==5.0.0a0
$ pip freeze | grep jupyter-collaboration
jupyter-collaboration==5.0.0a0
jupyter-collaboration-ui==3.0.0a0
$ pytest
============================= test session starts ==============================
platform linux -- Python 3.14.6, pytest-9.1.1, pluggy-1.6.0
rootdir: /home/brichet/projects/jupyter-chat
configfile: pyproject.toml
plugins: cov-7.1.0, anyio-4.14.1, asyncio-1.4.0
asyncio: mode=Mode.STRICT, debug=False, asyncio_default_fixture_loop_scope=None, asyncio_default_test_loop_scope=function
collected 22 items
python/jupyterlab-chat/jupyterlab_chat/tests/test_imports.py . [ 4%]
python/jupyterlab-chat/jupyterlab_chat/tests/test_ychat.py ............. [ 63%]
..... [ 86%]
python/jupyterlab-chat/jupyterlab_chat/tests/test_ychat_gc.py FFF [100%]
=================================== FAILURES ===================================
_____________________ test_ychat_collected_after_unobserve _____________________
def test_ychat_collected_after_unobserve():
"""YChat must be garbage-collected after unobserve().
Observers registered in __init__ are bound methods that capture ``self``.
If unobserve() does not remove them, the YChat is never collected (leak).
"""
ychat = YChat()
ychat.unobserve()
ref = weakref.ref(ychat)
del ychat
gc.collect()
> assert ref() is None, "YChat leaked after unobserve() (observers not removed)"
E AssertionError: YChat leaked after unobserve() (observers not removed)
E assert <jupyterlab_chat.ychat.YChat object at 0x7160197f3e90> is None
E + where <jupyterlab_chat.ychat.YChat object at 0x7160197f3e90> = <weakref at 0x7160193d4cc0; to 'jupyterlab_chat.ychat.YChat' at 0x7160197f3e90>()
python/jupyterlab-chat/jupyterlab_chat/tests/test_ychat_gc.py:26: AssertionError
______________ test_ychat_collected_after_observe_then_unobserve _______________
def test_ychat_collected_after_observe_then_unobserve():
"""YChat must be collectable after the realistic observe()/unobserve() flow.
A server registers a sync callback via ``observe()`` and later tears the
document down with ``unobserve()``. Both the ``observe()`` subscriptions and
the ``__init__`` observers must be removed for the YChat to be collected.
"""
ychat = YChat()
ychat.observe(lambda *args: None)
ychat.unobserve()
ref = weakref.ref(ychat)
del ychat
gc.collect()
> assert ref() is None, "YChat leaked after observe() then unobserve()"
E AssertionError: YChat leaked after observe() then unobserve()
E assert <jupyterlab_chat.ychat.YChat object at 0x716019e05cd0> is None
E + where <jupyterlab_chat.ychat.YChat object at 0x716019e05cd0> = <weakref at 0x71601985c310; to 'jupyterlab_chat.ychat.YChat' at 0x716019e05cd0>()
python/jupyterlab-chat/jupyterlab_chat/tests/test_ychat_gc.py:44: AssertionError
_______________ test_ychat_collected_after_unobserve_while_dirty _______________
def test_ychat_collected_after_unobserve_while_dirty():
"""YChat must be collectable even if torn down before any state event fires.
When the document is still dirty, ``_initialize`` has not yet removed the
state subscription, so ``unobserve()`` must remove it defensively.
"""
ychat = YChat()
assert ychat.dirty is True
ychat.unobserve()
ref = weakref.ref(ychat)
del ychat
gc.collect()
> assert ref() is None, "YChat leaked after unobserve() while dirty"
E AssertionError: YChat leaked after unobserve() while dirty
E assert <jupyterlab_chat.ychat.YChat object at 0x7160193f81d0> is None
E + where <jupyterlab_chat.ychat.YChat object at 0x7160193f81d0> = <weakref at 0x71601982fb00; to 'jupyterlab_chat.ychat.YChat' at 0x7160193f81d0>()
python/jupyterlab-chat/jupyterlab_chat/tests/test_ychat_gc.py:61: AssertionError
=========================== short test summary info ============================
FAILED python/jupyterlab-chat/jupyterlab_chat/tests/test_ychat_gc.py::test_ychat_collected_after_unobserve - AssertionError: YChat leaked after unobserve() (observers not removed)
FAILED python/jupyterlab-chat/jupyterlab_chat/tests/test_ychat_gc.py::test_ychat_collected_after_observe_then_unobserve - AssertionError: YChat leaked after observe() then unobserve()
FAILED python/jupyterlab-chat/jupyterlab_chat/tests/test_ychat_gc.py::test_ychat_collected_after_unobserve_while_dirty - AssertionError: YChat leaked after unobserve() while dirty
========================= 3 failed, 19 passed in 1.22s =========================| # Import jupyter_ydoc first to avoid a circular import via its entry points. | ||
| import jupyter_ydoc # noqa: F401 |
| # Only clear the subscriptions registered by a previous observe() call; | ||
| # the observers registered in __init__ must persist for the lifetime of | ||
| # the document (they are removed by unobserve() at teardown). | ||
| super().unobserve() |
There was a problem hiding this comment.
Out of curiosity (not related to the change), because I did not realize that before this PR, does it mean that only one callback can be attached to the whole document change (probably the document room) ?
What would happen if another extension do ychat.observe(callback) ?
Summary
YChat.__init__()registers an observer (_on_messages_change, plus the stateobserver
_initialize) whose callback is a bound method holding a reference toself.unobserve()did not remove these, so theYChatobject was nevergarbage collected after
unobserve()— leaking the document and everything itreferences.
This stores the messages subscription and adds a
YChat.unobserve()overridethat removes both
__init__observers (in addition to theobserve()subscriptions cleared by the base class). Adds a GC regression test.
Changes
__init__as_ymessages_subscription.YChat.unobserve()override: callsuper().unobserve(), then remove_ymessages_subscriptionand_ystate_subscription(guarded againstdouble-unobserve by nulling out after removal).
_initialize()now nulls_ystate_subscriptionafter unobserving, sounobserve()doesn't try to remove it twice.observe()now callssuper().unobserve()(instead ofself.unobserve()) sore-registering
observe()subscriptions no longer strips the__init__observers that the document needs for its lifetime.
tests/test_ychat_gc.pywith weakref-based GC regression tests covering:plain
unobserve(),observe()+unobserve(), and teardown while stilldirty.