fix plugin behavior in case of failure durng controller and item instantiations

bliss/config/plugins/bliss_controller.py

@@ -5,6 +5,7 @@
 # Copyright (c) 2015-2020 Beamline Control Unit, ESRF
 # Distributed under the GNU LGPLv3. See LICENSE for more info.
 
+from unittest.mock import Base
 from bliss.config.plugins.utils import find_top_class_and_node
 from bliss.controllers.bliss_controller import BlissController
 
@@ -36,7 +37,7 @@ def create_objects_from_config_node(cfg_obj, cfg_node):
     # always create the bliss controller first
     bctrl = klass(ctrl_node)
 
-    # print(f"\n=== From config: {item_name} from {bctrl.name}")
+    print(f"\n=== From config: {item_name} from {bctrl.name}")
 
     if isinstance(bctrl, BlissController):
 
@@ -46,25 +47,31 @@ def create_objects_from_config_node(cfg_obj, cfg_node):
         cacheditemnames2ctrl = bctrl._prepare_subitems_configs()
         # print(f"\n=== Caching: {list(cacheditemnames2ctrl.keys())} from {bctrl.name}")
 
-        # --- add the controller to registered items, if it has a name.
+        # --- add the controller to registered items (if it has a name)
         name2items = {}
         if ctrl_name:
             name2items[ctrl_name] = bctrl
 
-        # update the config cache dict now to avoid cyclic instanciation with internal references
+        # update the config cache dict now to avoid cyclic instantiation with internal references
         # an internal reference occurs when a subitem config uses a reference to another subitem owned by the same controller.
         yield name2items, cacheditemnames2ctrl
 
         # load config and init controller
-        bctrl._controller_init()
-
-        # --- don't forget to instanciate the object for which this function has been called (if not a controller)
+        try:
+            bctrl._controller_init()
+        except BaseException:
+            # remove cached obj if controller initialization fails (to avoid items with different instances of the same controller)
+            for iname in cacheditemnames2ctrl.keys():
+                cfg_obj._name2cache.pop(iname, None)
+            raise
+
+        # --- don't forget to instantiate the object for which this function has been called (if not a controller)
         if item_name != ctrl_name:
             obj = cfg_obj.get(item_name)
             yield {item_name: obj}
 
         # --- Now any new object_name going through 'config.get( obj_name )' should call 'create_object_from_cache' only.
-        # --- 'create_objects_from_config_node' should never be called again for any object related to the controller instanciated here (see config.get code)
+        # --- 'create_objects_from_config_node' should never be called again for any object related to the controller instantiated here (see config.get code)
 
     elif (
         item_name == ctrl_name
@@ -76,5 +83,11 @@ def create_objects_from_config_node(cfg_obj, cfg_node):
 
 
 def create_object_from_cache(config, name, bctrl):
-    # print(f"\n=== From cache: {name} from {bctrl.name}")
-    return bctrl._get_subitem(name)
+    print(f"\n=== From cache: {name} from {bctrl.name}")
+
+    try:
+        return bctrl._get_subitem(name)
+    except BaseException:
+        # put back item in cached items if instantiation has failed
+        config._name2cache[name] = bctrl
+        raise

bliss/config/static.py

@@ -1027,7 +1027,9 @@ class Config(metaclass=Singleton):
                 module_name = "default"
             m = __import__("bliss.config.plugins.%s" % (module_name), fromlist=[None])
             if hasattr(m, "create_object_from_cache"):
-                cache_object = self._name2cache.pop(name, None)
+                cache_object = self._name2cache.pop(
+                    name, None
+                )  # it should be popped only if object instantiation succeed !
                 if cache_object is not None:
                     cache_func = getattr(m, "create_object_from_cache")
                     instance_object = cache_func(self, name, cache_object)

tests/motors/test_initialization.py

@@ -126,76 +126,35 @@ def test_broken_controller_init(default_session):
         with pytest.raises(RuntimeError, match="FAILED TO INITIALIZE"):
             default_session.config.get("roby")
 
-        # === now config._name2instance is still empty because _controller_init() has failed (from_config did not yield bctrl)
-        # === but config._name2cache has been filled with bctrl items
+        # === now config._name2instance is still empty because _controller_init() has failed and roby was not instanciated
+        # === config._name2cache is also empty because cacheditems have been removed when _controller_init() has failed
         print("=== match=FAILED TO INITIALIZE")
         print("=== name2instance:", list(config._name2instance.keys()))
         print("=== name2cache:", list(config._name2cache.keys()))
         assert list(config._name2instance.keys()) == []
-        assert "roby" in list(config._name2cache.keys())
-        assert "robu" in list(config._name2cache.keys())
-        roby_cached_id = id(config._name2cache["roby"])  # store id for later comparison
-        robu_cached_id = id(
-            config._name2cache["robu"]
-        )  # check that another item share the same controller
-        assert roby_cached_id == robu_cached_id
-
-        # === expecting failure during plugin.from_cache => BlissController._get_subitem => __build_subitem_from_config()
-        # === because controller is not initialized properly (see bctrl.__ctrl_is_initialized)
+        assert list(config._name2cache.keys()) == []
+
+        # === expecting same failure during plugin.from_config => BlissController._controller_init()
         with pytest.raises(
-            RuntimeError, match="Controller not initialized"
+            RuntimeError, match="FAILED TO INITIALIZE"
         ):  # Controller is disabled
             default_session.config.get("roby")
 
-        # === now config._name2instance is still empty because roby has failed again
-        # === but roby has been remmoved from config._name2cache (during from_cache(roby) at previous step)
-        print("=== match=Controller not initialized")
+        # === now config._name2instance is still empty because _controller_init() has failed and roby was not instanciated
+        # === config._name2cache is also empty because cacheditems have been removed when _controller_init() has failed
+        print("=== match=FAILED TO INITIALIZE")
         print("=== name2instance:", list(config._name2instance.keys()))
         print("=== name2cache:", list(config._name2cache.keys()))
         assert list(config._name2instance.keys()) == []
-        assert "roby" not in list(config._name2cache.keys())
-        assert "robu" in list(config._name2cache.keys())
-
-        # === now roby is not cached anymore so the controller will be initialized again
-        # === so expecting failure during plugin.from_config => BlissController._controller_init()
-        with pytest.raises(RuntimeError, match="FAILED TO INITIALIZE"):
-            default_session.config.get("roby")
-
-        # === and now controller items are cached again but pointing to the new controller instance
-        new_roby_cached_id = id(config._name2cache["roby"])
-        new_robu_cached_id = id(config._name2cache["robu"])
-        assert new_roby_cached_id != roby_cached_id
-        assert new_robu_cached_id != robu_cached_id
+        assert list(config._name2cache.keys()) == []
 
     with mock.patch(
         "bliss.controllers.motors.mockup.Mockup.initialize_hardware",
         wraps=faulty_initialize,
     ):
 
-        # === now roby is cached with a faulty controller
-        print("=== now roby is cached with a faulty controller")
-        print("=== name2instance:", list(config._name2instance.keys()))
-        print("=== name2cache:", list(config._name2cache.keys()))
-        assert list(config._name2instance.keys()) == []
-        assert "roby" in list(config._name2cache.keys())
-        assert "robu" in list(config._name2cache.keys())
-        assert config._name2cache["roby"]._is_initialized == False
-
-        # === expecting failure during plugin.from_cache => BlissController._get_subitem => __build_subitem_from_config()
-        # === because controller is not initialized properly (see bctrl.__ctrl_is_initialized)
-        with pytest.raises(RuntimeError, match="Controller not initialized"):
-            roby = default_session.config.get("roby")
-
-        # === roby has been removed from cache but still not instantiated
-        print("=== now roby has been removed from cache but still not instantiated")
-        print("=== name2instance:", list(config._name2instance.keys()))
-        print("=== name2cache:", list(config._name2cache.keys()))
-        assert list(config._name2instance.keys()) == []
-        assert "roby" not in list(config._name2cache.keys())
-        assert "robu" in list(config._name2cache.keys())
-
-        # === so controller will be initialized again but successfully this time
-        # === and roby will be initialized too but with faulty controller hardware
+        # === now controller will be successfully initialized
+        # === and roby will be initialized too (because with the lazy init faulty hardware is not seen yet)
         roby = default_session.config.get("roby")
         assert roby
         assert roby._disabled == False  # not yet disabled because of lasy hardware init
@@ -205,12 +164,11 @@ def test_broken_controller_init(default_session):
         print("=== assert roby True")
         print("=== name2instance:", list(config._name2instance.keys()))
         print("=== name2cache:", list(config._name2cache.keys()))
-        assert id(roby.controller) != new_roby_cached_id  # ctrl has been created again
-        assert (
-            len(list(config._name2instance.keys())) == 1
-        )  # only roby because ctrl has no name else 2
         assert "roby" in list(config._name2instance.keys())
-        assert "roby" not in list(config._name2cache.keys())
+        assert "roby" not in list(
+            config._name2cache.keys()
+        )  # roby has been has been initialized and the removed from config cached items
+        assert "robu" in list(config._name2cache.keys())  # other items have been cached
 
         # === accessing position will force controller to init its hardware and will fail
         with pytest.raises(RuntimeError, match="FAILED TO INITIALIZE"):
@@ -221,13 +179,14 @@ def test_broken_controller_init(default_session):
 
     # roby and robu are on the same controller ;
     # controller is disabled because hardware init has failed
-    assert "robu" in list(config._name2cache.keys())
     with pytest.raises(RuntimeError, match="Controller is disabled"):
         default_session.config.get("robu")
-    assert "robu" not in list(config._name2instance.keys())
     assert "robu" not in list(
+        config._name2instance.keys()
+    )  # because robu instantiation has failed
+    assert "robu" in list(
         config._name2cache.keys()
-    )  # robu has been removed from cache
+    )  # robu has been put back in cache because instantiation has failed
 
     with pytest.raises(RuntimeError, match="Axis roby is disabled"):
         # axis is already disabled
@@ -240,8 +199,7 @@ def test_broken_controller_init(default_session):
     assert roby._disabled == True
     assert roby.controller._disabled == True
 
-    # === now controller is initialized but disabled
-    # === so call _init() to re-enable the controller
+    # === call _init() to re-enable the controller
     roby.controller._init()
     assert roby.controller._disabled == False
     roby2 = default_session.config.get("roby")
@@ -255,14 +213,6 @@ def test_broken_controller_init(default_session):
     robu = default_session.config.get("robu")
     robu.position
 
-    # Note: the re-creation of the controller after 2 consecutive failures with config.get('roby') is dangerous
-    # If you get roby ok (robu in cache) and then hardware fails while getting robu
-    # then if you try to get robu again it will recache roby with new ctrl instance
-
-    # the best would be to keep a single instance of the ctrl and forget about cached items in plugin
-    # actually the controller already cache its items, so plugin just ask to controller if it exist and
-    # even if it has failed to do bctrl._controller_init()
-
 
 def test_encoder_disable_broken_init(default_session):
     def faulty_initialize(*args, **kwargs):