From d66bfaf1d8d02b4fb55903be30528d2329953cc1 Mon Sep 17 00:00:00 2001 From: Matt Vollrath Date: Tue, 7 Apr 2020 04:58:19 -0400 Subject: [PATCH 1/3] Lock access to SubscriberManager public methods Prevent subscribe during unsubscribe critical section. --- .../rosbridge_library/internal/subscribers.py | 25 +++++++++++-------- 1 file changed, 14 insertions(+), 11 deletions(-) diff --git a/rosbridge_library/src/rosbridge_library/internal/subscribers.py b/rosbridge_library/src/rosbridge_library/internal/subscribers.py index 2be655ead..a7589447c 100644 --- a/rosbridge_library/src/rosbridge_library/internal/subscribers.py +++ b/rosbridge_library/src/rosbridge_library/internal/subscribers.py @@ -186,6 +186,7 @@ class SubscriberManager(): """ def __init__(self): + self._lock = Lock() self._subscribers = {} def subscribe(self, client_id, topic, callback, msg_type=None): @@ -198,13 +199,14 @@ def subscribe(self, client_id, topic, callback, msg_type=None): msg_type -- (optional) the type of the topic """ - if not topic in self._subscribers: - self._subscribers[topic] = MultiSubscriber(topic, msg_type) + with self._lock: + if not topic in self._subscribers: + self._subscribers[topic] = MultiSubscriber(topic, msg_type) - if msg_type is not None: - self._subscribers[topic].verify_type(msg_type) + if msg_type is not None: + self._subscribers[topic].verify_type(msg_type) - self._subscribers[topic].subscribe(client_id, callback) + self._subscribers[topic].subscribe(client_id, callback) def unsubscribe(self, client_id, topic): """ Unsubscribe from a topic @@ -214,14 +216,15 @@ def unsubscribe(self, client_id, topic): topic -- the topic to unsubscribe from """ - if not topic in self._subscribers: - return + with self._lock: + if not topic in self._subscribers: + return - self._subscribers[topic].unsubscribe(client_id) + self._subscribers[topic].unsubscribe(client_id) - if not self._subscribers[topic].has_subscribers(): - self._subscribers[topic].unregister() - del self._subscribers[topic] + if not self._subscribers[topic].has_subscribers(): + self._subscribers[topic].unregister() + del self._subscribers[topic] manager = SubscriberManager() From 1120717754cb02d07ff6270b4e6bf696c6bc0d62 Mon Sep 17 00:00:00 2001 From: Matt Vollrath Date: Tue, 7 Apr 2020 05:01:37 -0400 Subject: [PATCH 2/3] Unsubscribing an unsubscribed topic is an error This branch must not be ignored. --- .../src/rosbridge_library/internal/subscribers.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/rosbridge_library/src/rosbridge_library/internal/subscribers.py b/rosbridge_library/src/rosbridge_library/internal/subscribers.py index a7589447c..aade96750 100644 --- a/rosbridge_library/src/rosbridge_library/internal/subscribers.py +++ b/rosbridge_library/src/rosbridge_library/internal/subscribers.py @@ -217,9 +217,6 @@ def unsubscribe(self, client_id, topic): """ with self._lock: - if not topic in self._subscribers: - return - self._subscribers[topic].unsubscribe(client_id) if not self._subscribers[topic].has_subscribers(): From 212898c4103fbd73401de60683c8469897e51671 Mon Sep 17 00:00:00 2001 From: Matt Vollrath Date: Tue, 7 Apr 2020 05:10:19 -0400 Subject: [PATCH 3/3] Cleanup some redundant syntax in subscribers impl --- .../src/rosbridge_library/internal/subscribers.py | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/rosbridge_library/src/rosbridge_library/internal/subscribers.py b/rosbridge_library/src/rosbridge_library/internal/subscribers.py index aade96750..152e7fae8 100644 --- a/rosbridge_library/src/rosbridge_library/internal/subscribers.py +++ b/rosbridge_library/src/rosbridge_library/internal/subscribers.py @@ -119,7 +119,6 @@ def verify_type(self, msg_type): if not ros_loader.get_message_class(msg_type) is self.msg_class: raise TypeConflictException(self.topic, self.msg_class._type, msg_type) - return def subscribe(self, client_id, callback): """ Subscribe the specified client to this subscriber. @@ -150,8 +149,7 @@ def unsubscribe(self, client_id): def has_subscribers(self): """ Return true if there are subscribers """ with self.lock: - ret = len(self.subscriptions) != 0 - return ret + return len(self.subscriptions) != 0 def callback(self, msg, callbacks=None): """ Callback for incoming messages on the rospy.Subscriber @@ -177,7 +175,6 @@ def callback(self, msg, callbacks=None): except Exception as exc: # Do nothing if one particular callback fails except log it logerr("Exception calling subscribe callback: %s", exc) - pass class SubscriberManager():