fix: Memory leak from streams rooted in the global registry - #6990
Conversation
`Stream.registry` is a `WeakKeyDictionary`, but its values were held strongly. A stream reaches its own source back through its subscribers, so the weak key was reachable from its own value and every source that ever had a stream became immortal, along with the data it displayed. Sources now own their streams on `LabelledData._streams` and the registry only indexes them weakly, so a source and its streams are collected together. `Stream.registry` keeps its read API, resolving the references on access. This also removes `_WeakSubscriber`, which cut the wrong edge. It only wrapped bound methods, so closure and lambda subscribers always leaked; holding the temporary `functools.partial` weakly stopped partial subscribers firing at all (#6945); and `__bool__` returning False for param methods made `cleanup()` drop subscribers belonging to other plots (#6988). With the global root gone, subscribers can simply be strong. Measured over 8 open/close cycles of the reported app, RSS grows 207 -> 596 MB before this change and stays flat at 211 MB after. Closes #6945 Closes #6988 Assisted-by: Claude Code:claude-opus-5
The `Plot.document` setter registers `doc.on_session_destroyed( self._session_destroy)`, and the document holds that callback, and through it the whole plot tree, strongly. `BokehPlot.cleanup` assigned `plot._document = None` directly rather than going through the setter, so the hook was never removed and the plot lived for as long as its document did. Outside a served session that document is the process wide `curdoc()`, which never goes away, so every plot ever rendered was retained. The deregistration is extracted into `Plot._unwatch_session` and called from the setter and from both `cleanup` implementations. The setter previously only dropped the old registration when the *new* document also qualified for one, so assigning `None` or a plain document leaked it. Assisted-by: Claude Code:claude-opus-5
Renderers are long lived singletons registered in `Store.renderers`, so the strong reference kept the most recently rendered plot, and the data it displays, alive for the rest of the process even after the plot had been cleaned up. `last_plot` becomes a property backed by a weak reference. Assignment is unchanged, and reading it returns None once the plot has gone. Assisted-by: Claude Code:claude-opus-5
`attach_streams` guarded against re-subscribing with `plot.refresh not in stream._subscribers`, but `_subscribers` holds `(precedence, subscriber)` pairs, so a bare method could never match and the guard never fired. A composite plot calls `attach_streams` once for itself and again for every element subplot it traverses, so an overlay of two DynamicMaps subscribed its `refresh` four times per stream and refreshed four times on every stream event. Comparing against `stream.subscribers`, which is the list of subscribers, makes the guard work. Assisted-by: Claude Code:claude-opus-5
`Stream.add_subscriber` documents precedence one and below as the user
range, with HoloViews reserving higher values, and `Stream.clear` splits
on that boundary. `_link_dimensioned_streams` registered `_stream_update`
at exactly one, so it sat in the user range and `clear("user")` dropped
it, leaving a composite plot whose title no longer updated on a
dimensioned stream event.
It now registers at 1.05: above the user range, and still below the 1.1
of the plot refresh registered by `attach_streams`, so the subscriber
ordering is unchanged.
Assisted-by: Claude Code:claude-opus-5
The three tests differed only in the policy and the expected remainder. Behaviour and expectations are unchanged. Assisted-by: Claude Code:claude-opus-5
952cb49 to
0b97360
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6990 +/- ##
==========================================
+ Coverage 89.33% 89.35% +0.01%
==========================================
Files 344 344
Lines 74689 74790 +101
==========================================
+ Hits 66727 66826 +99
- Misses 7962 7964 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
0b97360 to
2afa83a
Compare
|
Asked Opus to review this branch Review
Review — PR #6990: "fix: Memory leak from streams rooted in the global registry"OverviewInverts stream ownership: the source element owns its streams ( The core design is right, and the claimed fixes were verified empirically against
Findings1. Non-
|
|
Great! I'm looking forward to seeing this included in a release. |
|
Is it possible for you to give this a go before merging? |
|
It's all good: the subscriber is executed AND the memory is freed! Thanks a bunch |
philippjfr
left a comment
There was a problem hiding this comment.
Looking at the code everything looks good to me, but it's hard to reason about so I've also done a bunch of testing.
Description
Closes #6945
Closes #6988
Alternative to #6970
The problem
Stream.registryused to be aWeakKeyDictionary, so an entry should disappear once the element does. It never did, because only the keys were weak. The values were ordinary references [self.registry[source] = [self], and a stream reaches its own source back through its subscribers:holoviews/holoviews/streams.py
Line 172 in fff801d
holoviews/holoviews/streams.py
Line 376 in fff801d
The key was reachable from its own value, so the entry never expired. Any element ever passed as
source=stayed alive until the process exited, along with its data, its streams and whatever those subscribers were bound to.#6875 tried to fix this by holding subscribers weakly, in
_WeakSubscriber. That cut the wrong link, and it only did anything for bound methods:checkrequiredinspect.ismethod, so closures and lambdas were neverwrapped and kept leaking
functools.partialpassed thecheckbut fell through toweakref.ref(subscriber). Nothing else referenced the partial, so it wascollected straight away and the subscriber never fired ([Regression] Stream subscribers based on partial functions no longer work #6945)
__bool__reported param methods as dead, so callingcleanup()on one plotremoved subscribers belonging to another (Plot.cleanup() severs other plots' subscribers on shared streams: owner filter defeated by _WeakSubscriber.__bool__ #6988)
What changed in this PR
Ownership was turned around. The source element now keeps its own streams in
LabelledData._streams, andStream.registrywas reduced to a weak index that only exists so a clone with the same_plot_idcan still find them. An element, its streams and their subscribers are now just a cycle, which the garbage collector frees once nothing else refers to it.With the global reference gone,
_WeakSubscriberwas deleted and subscribers went back to being ordinary strong references. #6945 and #6988 stopped being possible rather than being worked around.A second leak with the same effect turned up while measuring the first. Every plot registers
on_session_destroyedon its document, and the document keeps that callback, and the whole plot with it.BokehPlot.cleanupsetplot._document = Nonedirectly instead of going through the setter, so nothing ever removed the callback.Outside a server that document is
curdoc(), which lives for the whole process, so every plot ever rendered was kept. The deregistration was pulled out intoPlot._unwatch_sessionand is now called from the setter and from bothcleanupimplementations.Also fixed
Renderer.last_plotheld the last plot for the life of the process. It is now a weak reference.attach_streamscomparedplot.refreshagainst the(precedence, subscriber)pairs, so its guard never matched and an overlay subscribed the same plot four times. It now compares againststream.subscribers._link_dimensioned_streamsregistered_stream_updateat precedence exactly one, inside the rangeStream.cleartreats as belonging to the user, soclear("user")dropped it. It registers at 1.05 now.AI Disclosure
Tool & Model: Claude Code:claude-opus-5
Usage:
Checklist