From 6d004c41449a84bfb00d1bbc9affdbd8b08f2fa8 Mon Sep 17 00:00:00 2001 From: armando-migliaccio Date: Tue, 11 Aug 2015 18:16:30 -0700 Subject: [PATCH] Improve callback registry devref documentation and usability Latest developments have revealed that the registry can be misused under certain circumstances, and that it can be harder to use by projects that extend Neutron. This patch improves the devref documentation so that developers know what to expect. Change-Id: I565b6a2f2a58bf22eae5b36f03c4fd24ba0774d2 --- doc/source/devref/callbacks.rst | 18 ++++++++++++++++++ neutron/callbacks/events.py | 16 +--------------- neutron/callbacks/manager.py | 19 +++++++++---------- neutron/callbacks/resources.py | 11 +---------- neutron/tests/unit/callbacks/test_manager.py | 17 +++++++---------- 5 files changed, 36 insertions(+), 45 deletions(-) diff --git a/doc/source/devref/callbacks.rst b/doc/source/devref/callbacks.rst index 71c85f80e..ff6cfc77e 100644 --- a/doc/source/devref/callbacks.rst +++ b/doc/source/devref/callbacks.rst @@ -300,6 +300,14 @@ The output is: FAQ === +Can I use the callbacks registry to subscribe and notify non-core resources and events? + + Short answer is yes. The callbacks module defines literals for what are considered core Neutron + resources and events. However, the ability to subscribe/notify is not limited to these as you + can use your own defined resources and/or events. Just make sure you use string literals, as + typos are common, and the registry does not provide any runtime validation. Therefore, make + sure you test your code! + What is the relationship between Callbacks and Taskflow? There is no overlap between Callbacks and Taskflow or mutual exclusion; as matter of fact they @@ -315,6 +323,16 @@ Is there any ordering guarantee during notifications? notified. Priorities can be a future extension, if a use case arises that require enforced ordering. +How is the the notifying object expected to interact with the subscribing objects? + + The ``notify`` method implements a one-way communication paradigm: the notifier sends a message + without expecting a response back (in other words it fires and forget). However, due to the nature + of Python, the payload can be mutated by the subscribing objects, and this can lead to unexpected + behavior of your code, if you assume that this is the intentional design. Bear in mind, that + passing-by-value using deepcopy was not chosen for efficiency reasons. Having said that, if you + intend for the notifier object to expect a response, then the notifier itself would need to act + as a subscriber. + Is the registry thread-safe? Short answer is no: it is not safe to make mutations while callbacks are being called (more diff --git a/neutron/callbacks/events.py b/neutron/callbacks/events.py index 2abc57ce1..7dfd83d5e 100644 --- a/neutron/callbacks/events.py +++ b/neutron/callbacks/events.py @@ -10,6 +10,7 @@ # License for the specific language governing permissions and limitations # under the License. +# String literals representing core events. BEFORE_CREATE = 'before_create' BEFORE_READ = 'before_read' BEFORE_UPDATE = 'before_update' @@ -27,18 +28,3 @@ ABORT_DELETE = 'abort_delete' ABORT = 'abort_' BEFORE = 'before_' - -VALID = ( - BEFORE_CREATE, - BEFORE_READ, - BEFORE_UPDATE, - BEFORE_DELETE, - AFTER_CREATE, - AFTER_READ, - AFTER_UPDATE, - AFTER_DELETE, - ABORT_CREATE, - ABORT_READ, - ABORT_UPDATE, - ABORT_DELETE, -) diff --git a/neutron/callbacks/manager.py b/neutron/callbacks/manager.py index 4927ff337..c5b97e9af 100644 --- a/neutron/callbacks/manager.py +++ b/neutron/callbacks/manager.py @@ -17,7 +17,6 @@ from oslo_utils import reflection from neutron.callbacks import events from neutron.callbacks import exceptions -from neutron.callbacks import resources from neutron.i18n import _LE LOG = logging.getLogger(__name__) @@ -40,13 +39,15 @@ class CallbacksManager(object): """ LOG.debug("Subscribe: %(callback)s %(resource)s %(event)s", {'callback': callback, 'resource': resource, 'event': event}) - if resource not in resources.VALID: - raise exceptions.Invalid(element='resource', value=resource) - if event not in events.VALID: - raise exceptions.Invalid(element='event', value=event) callback_id = _get_id(callback) - self._callbacks[resource][event][callback_id] = callback + try: + self._callbacks[resource][event][callback_id] = callback + except KeyError: + # Initialize the registry for unknown resources and/or events + # prior to enlisting the callback. + self._callbacks[resource][event] = {} + self._callbacks[resource][event][callback_id] = callback # We keep a copy of callbacks to speed the unsubscribe operation. if callback_id not in self._index: self._index[callback_id] = collections.defaultdict(set) @@ -125,9 +126,6 @@ class CallbacksManager(object): """Brings the manager to a clean slate.""" self._callbacks = collections.defaultdict(dict) self._index = collections.defaultdict(dict) - for resource in resources.VALID: - for event in events.VALID: - self._callbacks[resource][event] = collections.defaultdict() def _notify_loop(self, resource, event, trigger, **kwargs): """The notification loop.""" @@ -135,8 +133,9 @@ class CallbacksManager(object): {'resource': resource, 'event': event}) errors = [] + callbacks = self._callbacks[resource].get(event, {}).items() # TODO(armax): consider using a GreenPile - for callback_id, callback in self._callbacks[resource][event].items(): + for callback_id, callback in callbacks: try: LOG.debug("Calling callback %s", callback_id) callback(resource, event, trigger, **kwargs) diff --git a/neutron/callbacks/resources.py b/neutron/callbacks/resources.py index d796faf49..1544fe5a4 100644 --- a/neutron/callbacks/resources.py +++ b/neutron/callbacks/resources.py @@ -10,6 +10,7 @@ # License for the specific language governing permissions and limitations # under the License. +# String literals representing core resources. PORT = 'port' ROUTER = 'router' ROUTER_GATEWAY = 'router_gateway' @@ -17,13 +18,3 @@ ROUTER_INTERFACE = 'router_interface' SECURITY_GROUP = 'security_group' SECURITY_GROUP_RULE = 'security_group_rule' SUBNET = 'subnet' - -VALID = ( - PORT, - ROUTER, - ROUTER_GATEWAY, - ROUTER_INTERFACE, - SECURITY_GROUP, - SECURITY_GROUP_RULE, - SUBNET, -) diff --git a/neutron/tests/unit/callbacks/test_manager.py b/neutron/tests/unit/callbacks/test_manager.py index e4e64323d..cdf32e020 100644 --- a/neutron/tests/unit/callbacks/test_manager.py +++ b/neutron/tests/unit/callbacks/test_manager.py @@ -13,7 +13,6 @@ # under the License. import mock -import testtools from neutron.callbacks import events from neutron.callbacks import exceptions @@ -44,15 +43,6 @@ class CallBacksManagerTestCase(base.BaseTestCase): callback_1.counter = 0 callback_2.counter = 0 - def test_subscribe_invalid_resource_raise(self): - with testtools.ExpectedException(exceptions.Invalid): - self.manager.subscribe(mock.ANY, 'foo_resource', mock.ANY) - - def test_subscribe_invalid_event_raise(self): - self.assertRaises(exceptions.Invalid, - self.manager.subscribe, - mock.ANY, mock.ANY, 'foo_event') - def test_subscribe(self): self.manager.subscribe( callback_1, resources.PORT, events.BEFORE_CREATE) @@ -60,6 +50,13 @@ class CallBacksManagerTestCase(base.BaseTestCase): self.manager._callbacks[resources.PORT][events.BEFORE_CREATE]) self.assertIn(callback_id_1, self.manager._index) + def test_subscribe_unknown(self): + self.manager.subscribe( + callback_1, 'my_resource', 'my-event') + self.assertIsNotNone( + self.manager._callbacks['my_resource']['my-event']) + self.assertIn(callback_id_1, self.manager._index) + def test_subscribe_is_idempotent(self): self.manager.subscribe( callback_1, resources.PORT, events.BEFORE_CREATE) -- 2.45.2