Repository: boto/botocore
Pull request #467 (2015-02-19 18:05:54)

Copy unique_id handlers for events
When event emitters were being copied they did not copy the dictionary of unique ids which affected registering handlers with unique ids. This pull request fixes that issue and renames ``_unique_id_cache`` to ``_unique_id_handlers`` to make it explicit that it is not a cache as opposed to a different representation of the handlers registered with respect to their unique id.

Fixes https://github.com/boto/botocore/issues/465

cc @jamesls @danielgtaylor 


Patch:
diff --git a/botocore/hooks.py b/botocore/hooks.py
index 131daf728..b6aec8ac9 100644
--- a/botocore/hooks.py
+++ b/botocore/hooks.py
@@ -170,7 +170,7 @@ class HierarchicalEmitter(BaseEventHooks):
         self._handlers = _PrefixTrie()
         # This is used to ensure that unique_id's are only
         # registered once.
-        self._unique_id_cache = {}
+        self._unique_id_handlers = {}
 
     def _emit(self, event_name, kwargs, stop_on_response=False):
         """
@@ -263,10 +263,10 @@ class HierarchicalEmitter(BaseEventHooks):
     def _register_section(self, event_name, handler, unique_id,
                           unique_id_uses_count, section):
         if unique_id is not None:
-            if unique_id in self._unique_id_cache:
+            if unique_id in self._unique_id_handlers:
                 # We've already registered a handler using this unique_id
                 # so we don't need to register it again.
-                count = self._unique_id_cache[unique_id].get('count', None)
+                count = self._unique_id_handlers[unique_id].get('count', None)
                 if unique_id_uses_count:
                     if not count:
                         raise ValueError("Initial registration of"
@@ -274,7 +274,7 @@ class HierarchicalEmitter(BaseEventHooks):
                             " Subsequent register calls to unique id must"
                             " specify use of a counter as well." % unique_id)
                     else:
-                        self._unique_id_cache[unique_id]['count'] += 1
+                        self._unique_id_handlers[unique_id]['count'] += 1
                 else:
                     if count:
                         raise ValueError("Initial registration of"
@@ -286,13 +286,13 @@ class HierarchicalEmitter(BaseEventHooks):
             else:
                 # Note that the trie knows nothing about the unique
                 # id.  We track uniqueness in this class via the
-                # _unique_id_cache.
+                # _unique_id_handlers.
                 self._handlers.append_item(event_name, handler,
                                            section=section)
-                unique_id_cache_item = {'handler': handler}
+                unique_id_handler_item = {'handler': handler}
                 if unique_id_uses_count:
-                    unique_id_cache_item['count'] = 1
-                self._unique_id_cache[unique_id] = unique_id_cache_item
+                    unique_id_handler_item['count'] = 1
+                self._unique_id_handlers[unique_id] = unique_id_handler_item
         else:
             self._handlers.append_item(event_name, handler, section=section)
         # Super simple caching strategy for now, if we change the registrations
@@ -303,7 +303,7 @@ class HierarchicalEmitter(BaseEventHooks):
                    unique_id_uses_count=False):
         if unique_id is not None:
             try:
-                count = self._unique_id_cache[unique_id].get('count', None)
+                count = self._unique_id_handlers[unique_id].get('count', None)
             except KeyError:
                 # There's no handler matching that unique_id so we have
                 # nothing to unregister.
@@ -315,9 +315,9 @@ class HierarchicalEmitter(BaseEventHooks):
                         " Subsequent unregister calls to unique id must"
                         " specify use of a counter as well." % unique_id)
                 elif count == 1:
-                    handler = self._unique_id_cache.pop(unique_id)['handler']
+                    handler = self._unique_id_handlers.pop(unique_id)['handler']
                 else:
-                    self._unique_id_cache[unique_id]['count'] -= 1
+                    self._unique_id_handlers[unique_id]['count'] -= 1
                     return
             else:
                 if count:
@@ -326,7 +326,7 @@ class HierarchicalEmitter(BaseEventHooks):
                         " Subsequent unregister calls to unique id must"
                         " specify not to use a counter as well." % 
                         unique_id)
-                handler = self._unique_id_cache.pop(unique_id)['handler']
+                handler = self._unique_id_handlers.pop(unique_id)['handler']
         try:
             self._handlers.remove_item(event_name, handler)
             self._lookup_cache = {}
@@ -337,6 +337,7 @@ class HierarchicalEmitter(BaseEventHooks):
         new_instance = self.__class__()
         new_state = self.__dict__.copy()
         new_state['_handlers'] = copy.copy(self._handlers)
+        new_state['_unique_id_handlers'] = copy.copy(self._unique_id_handlers)
         new_instance.__dict__ = new_state
         return new_instance
 


Test patch:
diff --git a/tests/unit/test_client.py b/tests/unit/test_client.py
index dceac58f6..b055ecb6e 100644
--- a/tests/unit/test_client.py
+++ b/tests/unit/test_client.py
@@ -594,3 +594,40 @@ class TestAutoGeneratedClient(unittest.TestCase):
         self.assertTrue(hasattr(service_client.meta, 'events'))
         # Sanity check the event emitter has an .emit() method.
         self.assertTrue(hasattr(service_client.meta.events, 'emit'))
+
+    def test_client_register_seperate_unique_id_event(self):
+        event_emitter = hooks.HierarchicalEmitter()
+        creator = self.create_client_creator(event_emitter=event_emitter)
+
+        client1 = creator.create_client('myservice', 'us-west-2')
+        client2 = creator.create_client('myservice', 'us-west-2')
+
+        def ping(**kwargs):
+            return 'foo'
+
+        client1.meta.events.register('some-event', ping, 'my-unique-id')
+        client2.meta.events.register('some-event', ping, 'my-unique-id')
+
+        # Ensure both clients can register a function with an unique id
+        client1_responses = client1.meta.events.emit('some-event')
+        self.assertEqual(len(client1_responses), 1)
+        self.assertEqual(client1_responses[0][1], 'foo')
+
+        client2_responses = client2.meta.events.emit('some-event')
+        self.assertEqual(len(client2_responses), 1)
+        self.assertEqual(client2_responses[0][1], 'foo')
+
+        # Ensure when a client is unregistered the other client has
+        # the unique-id event still registered.
+        client1.meta.events.unregister('some-event', ping, 'my-unique-id')
+        client1_responses = client1.meta.events.emit('some-event')
+        self.assertEqual(len(client1_responses), 0)
+
+        client2_responses = client2.meta.events.emit('some-event')
+        self.assertEqual(len(client2_responses), 1)
+        self.assertEqual(client2_responses[0][1], 'foo')
+
+        # Ensure that the other client can unregister the event
+        client2.meta.events.unregister('some-event', ping, 'my-unique-id')
+        client2_responses = client2.meta.events.emit('some-event')
+        self.assertEqual(len(client2_responses), 0)
diff --git a/tests/unit/test_hooks.py b/tests/unit/test_hooks.py
index 6c3c8179a..9040cfc72 100644
--- a/tests/unit/test_hooks.py
+++ b/tests/unit/test_hooks.py
@@ -507,6 +507,46 @@ class TestWildcardHandlers(unittest.TestCase):
         self.emitter.emit('foo.bar.baz', id_name='last-time')
         self.assertEqual(second, ['third-time'])
 
+    def test_copy_emitter_with_unique_id_event(self):
+        # Here we're not testing copy directly, we're testing
+        # the observable behavior from copying an event emitter.
+        first = []
+        def first_handler(id_name, **kwargs):
+            first.append(id_name)
+
+        second = []
+        def second_handler(id_name, **kwargs):
+            second.append(id_name)
+
+        self.emitter.register('foo', first_handler, 'bar')
+        self.emitter.emit('foo', id_name='first-time')
+        self.assertEqual(first, ['first-time'])
+        self.assertEqual(second, [])
+
+        copied_emitter = copy.copy(self.emitter)
+
+        # If we register an event handler with the copied
+        # emitter, the event should not get registered again
+        # because the unique id was already used.
+        copied_emitter.register('foo', second_handler, 'bar')
+        copied_emitter.emit('foo', id_name='second-time')
+        self.assertEqual(first, ['first-time', 'second-time'])
+        self.assertEqual(second, [])
+
+        # If we unregister the first event from the copied emitter,
+        # We should be able to register the second handler.
+        copied_emitter.unregister('foo', first_handler, 'bar')
+        copied_emitter.register('foo', second_handler, 'bar')
+        copied_emitter.emit('foo', id_name='third-time')
+        self.assertEqual(first, ['first-time', 'second-time'])
+        self.assertEqual(second, ['third-time'])
+
+        # The original event emitter should have the unique id event still
+        # registered though.
+        self.emitter.emit('foo', id_name='fourth-time')
+        self.assertEqual(first, ['first-time', 'second-time', 'fourth-time'])
+        self.assertEqual(second, ['third-time'])
+
     def test_copy_events_with_partials(self):
         # There's a bug in python2.6 where you can't deepcopy
         # a partial object.  We want to ensure that doesn't