Change from using Widget._instances class variable to the global _widget_instances to fix a Segmentation Fault that was causing test failure.

Also added the static method Widget.all_widgets that returns a copy of the current widgets.

Author: Alan

python/ipywidgets/ipywidgets/embed.py

@@ -129,7 +129,7 @@ def _get_recursive_state(widget, store=None, drop_defaults=False):
 
 def add_resolved_links(store, drop_defaults):
     """Adds the state of any link models between two models in store"""
-    for widget_id, widget in Widget._instances.items(): # go over all widgets
+    for widget_id, widget in Widget.all_widgets().items():  # go over all widgets
         if isinstance(widget, Link) and widget_id not in store:
             if widget.source[0].model_id in store and widget.target[0].model_id in store:
                 store[widget.model_id] = widget._get_embed_state(drop_defaults=drop_defaults)
@@ -207,7 +207,7 @@ def embed_data(views, drop_defaults=True, state=None):
         view_specs: a list of widget view specs
     """
     if views is None:
-        views = [w for w in Widget._instances.values() if isinstance(w, DOMWidget)]
+        views = [w for w in Widget.all_widgets().values() if isinstance(w, DOMWidget)]
     else:
         try:
             views[0]

python/ipywidgets/ipywidgets/tests/test_embed.py

@@ -29,7 +29,7 @@ class CaseWidget(Widget):
 class TestEmbed:
 
     def teardown(self):
-        for w in tuple(Widget._instances.values()):
+        for w in tuple(Widget.all_widgets().values()):
             w.close()
 
     def test_embed_data_simple(self):

python/ipywidgets/ipywidgets/widgets/tests/test_widget.py

@@ -60,12 +60,12 @@ def test_close_all():
     # create a couple of widgets
     widgets = [Button() for i in range(10)]
 
-    assert len(Widget._instances) > 0, "expect active widgets"
-    assert Widget._instances[widgets[0].model_id] is widgets[0]
+    assert len(Widget.all_widgets()) > 0, "expect active widgets"
+    assert Widget.all_widgets()[widgets[0].model_id] is widgets[0]
     # close all the widgets
     Widget.close_all()
 
-    assert len(Widget._instances) == 0, "active widgets should be cleared"
+    assert len(Widget.all_widgets()) == 0, "active widgets should be cleared"
 
 
 def test_widget_copy():
@@ -79,12 +79,12 @@ def test_widget_copy():
 def test_widget_open():
     button = Button()
     model_id = button.model_id
-    assert model_id in Widget._instances
+    assert model_id in Widget.all_widgets()
     spec = button.get_view_spec()
     assert list(spec) == ["version_major", "version_minor", "model_id"]
     assert spec["model_id"]
     button.close()
-    assert model_id not in Widget._instances
+    assert model_id not in Widget.all_widgets()
     with pytest.raises(RuntimeError, match="Widget is closed"):
         button.open()
     with pytest.raises(RuntimeError, match="Widget is closed"):
@@ -159,28 +159,33 @@ def test_widget_open():
         "Widget",
     ],
 )
-def test_weakreference(class_name):
+@pytest.mark.parametrize("enable_weakref", [True, False])
+def test_weakreference(class_name, enable_weakref):
     # Ensure the base instance of all widgets can be deleted / garbage collected.
-    ipw.enable_weakreference()
+    if enable_weakref:
+        ipw.enable_weakreference()
+    cls = getattr(ipw, class_name)
+    if class_name in ["SelectionRangeSlider", "SelectionSlider"]:
+        kwgs = {"options": [1, 2, 4]}
+    else:
+        kwgs = {}
     try:
-        cls = getattr(ipw, class_name)
-        if class_name in ["SelectionRangeSlider", "SelectionSlider"]:
-            kwgs = {"options": [1, 2, 4]}
-        else:
-            kwgs = {}
-        w: Widget = cls(**kwgs)
+        w = cls(**kwgs)
         deleted = False
         def on_delete():
             nonlocal deleted
             deleted = True
         weakref.finalize(w, on_delete)
         # w should be the only strong ref to the widget.
         # calling `del` should invoke its immediate deletion calling the `__del__` method.
+        if not enable_weakref:
+            w.close()
         del w
         gc.collect()
         assert deleted
     finally:
-        ipw.disable_weakreference()
+        if enable_weakref:
+            ipw.disable_weakreference()
 
 
 @pytest.mark.parametrize("weakref_enabled", [True, False])
@@ -201,7 +206,7 @@ def my_click(self, b):
         b = TestButton(description="button")
         weakref.finalize(b, on_delete)
         b_ref = weakref.ref(b)
-        assert b in Widget._instances.values()
+        assert b in Widget.all_widgets().values()
 
         b.on_click(b.my_click)
         b.on_click(lambda x: setattr(x, "clicked", True))
@@ -211,11 +216,11 @@ def my_click(self, b):
 
         if weakref_enabled:
             ipw.enable_weakreference()
-            assert b in Widget._instances.values(), "Instances not transferred"
+            assert b in Widget.all_widgets().values(), "Instances not transferred"
             ipw.disable_weakreference()
-            assert b in Widget._instances.values(), "Instances not transferred"
+            assert b in Widget.all_widgets().values(), "Instances not transferred"
             ipw.enable_weakreference()
-            assert b in Widget._instances.values(), "Instances not transferred"
+            assert b in Widget.all_widgets().values(), "Instances not transferred"
 
         b.click()
         assert click_count == 2
@@ -227,7 +232,7 @@ def my_click(self, b):
             assert deleted
         else:
             assert not deleted
-            assert b_ref() in Widget._instances.values()
+            assert b_ref() in Widget.all_widgets().values()
             b_ref().close()
             gc.collect()
             assert deleted, "Closing should remove the last strong reference."

python/ipywidgets/ipywidgets/widgets/tests/test_widget_box.py

@@ -55,3 +55,30 @@ def test_box_validate_mode():
     with pytest.raises(TraitError):
         box.children += ("Not a widget", closed_button)
 
+
+def test_box_gc():
+    widgets.enable_weakreference()
+    # Test Box gc collected and children lifecycle managed.
+    try:
+        deleted = False
+
+        class TestButton(widgets.Button):
+            def my_click(self, b):
+                pass
+
+        button = TestButton(description="button")
+        button.on_click(button.my_click)
+
+        b = widgets.VBox(children=[button])
+
+        def on_delete():
+            nonlocal deleted
+            deleted = True
+
+        weakref.finalize(b, on_delete)
+        del b
+        gc.collect()
+        assert deleted
+    finally:
+        pass
+        widgets.disable_weakreference()

python/ipywidgets/ipywidgets/widgets/widget.py

@@ -6,7 +6,6 @@
 in the Jupyter notebook front-end.
 """
 import os
-import typing
 import weakref
 from contextlib import contextmanager
 from collections.abc import Iterable
@@ -42,23 +41,33 @@ def envset(name, default):
 # for a discussion on using weak references see:
 #  https://github.com/jupyter-widgets/ipywidgets/issues/1345
 
+# A global to store all widget instances so that it can be swapped out when
+# enabling/disabling weakreferences
+_widget_instances = {}
+
+
 def enable_weakreference():
-    """Use a WeakValueDictionary to store references to Widget instances.
+    """Configure the module to only maintain a weakreference between the comm_id and widget for all widgets.
 
-    A strong reference must be kept to widgets. 
+    With this enabled the user must maintain a strong reference to widgets. The
+    advantage being that memory leaks prevented in long running programs.
     """
-    if not isinstance(Widget._instances, weakref.WeakValueDictionary):
-        Widget._instances = weakref.WeakValueDictionary(Widget._instances)
+    global _widget_instances
+    if not isinstance(_widget_instances, weakref.WeakValueDictionary):
+        _widget_instances = weakref.WeakValueDictionary(_widget_instances)
+        # Widget.__dict__["_instances"] = weakref.WeakValueDictionary()
+
 
 def disable_weakreference():
-    """Use a standard dictionary to store references to Widget instances (default behavior).
+    """Configure the module to only maintain a strong reference between the comm_id and widget for all widgets.
 
-    Note: this is the default setting and maintains a strong reference to the
-    the widget preventing automatic garbage collection. When the widget is closed
-    it can be garbage collected.
+    !!! Note:
+        This is the default behavior. The method `Widget.close` should be called when it is no longer required.
     """
-    if isinstance(Widget._instances, weakref.WeakValueDictionary):
-        Widget._instances = dict(Widget._instances)
+    global _widget_instances
+    if isinstance(_widget_instances, weakref.WeakValueDictionary):
+        _widget_instances = dict(_widget_instances)
+
 
 def _widget_to_json(x, obj):
     if isinstance(x, Widget):
@@ -75,8 +84,11 @@ def _json_to_widget(x, obj):
         return {k: _json_to_widget(v, obj) for k, v in x.items()}
     elif isinstance(x, (list, tuple)):
         return [_json_to_widget(v, obj) for v in x]
-    elif isinstance(x, str) and x.startswith("IPY_MODEL_") and x[10:] in Widget._instances:
-        return Widget._instances[x[10:]]
+    elif isinstance(x, str) and x.startswith("IPY_MODEL_"):
+        try:
+            return _widget_instances[x[10:]]
+        except (KeyError, IndexError):
+            pass
     else:
         return x
 
@@ -308,13 +320,18 @@ class Widget(LoggingHasTraits):
     _widget_construction_callback = None
     _control_comm = None
 
-    _instances: typing.ClassVar[typing.MutableMapping[str, "Widget"]] = {}
 
     @classmethod
     def close_all(cls):
-        for widget in list(Widget._instances.values()):
+        while _widget_instances:
+            _, widget = _widget_instances.popitem()
             widget.close()
 
+    @staticmethod
+    def all_widgets() -> dict[str, "Widget"]:
+        "Returns a dict mapping `comm_id` to Widget of all Widget instances."
+        return dict(_widget_instances)
+
     @staticmethod
     def on_widget_constructed(callback):
         """Registers a callback to be called when a widget is constructed.
@@ -354,7 +371,7 @@ def _handle_control_comm_msg(cls, msg):
         if method == 'request_states':
             # Send back the full widgets state
             cls.get_manager_state()
-            widgets = cls._instances.values()
+            widgets = _widget_instances.values()
             full_state = {}
             drop_defaults = False
             for widget in widgets:
@@ -405,7 +422,7 @@ def get_manager_state(cls, drop_defaults=False, widgets=None):
         """
         state = {}
         if widgets is None:
-            widgets = cls._instances.values()
+            widgets = _widget_instances.values()
         for widget in widgets:
             state[widget.model_id] = widget._get_embed_state(drop_defaults=drop_defaults)
         return {'version_major': 2, 'version_minor': 0, 'state': state}
@@ -516,9 +533,10 @@ def _comm_changed(self, change):
         if change['old']:
             change['old'].on_msg(None)
             change['old'].close()
-            self._instances.pop(change['old'].comm_id, None)
+            if _widget_instances:  # This check is needed to avoid errors on cleanup
+                _widget_instances.pop(change["old"].comm_id, None)
         if change['new']:
-            self._instances[change["new"].comm_id] = self
+            _widget_instances[change["new"].comm_id] = self
             self._model_id = change["new"].comm_id
 
             # prevent memory leaks by using a weak reference to self.