Log a one-shot warning instead of exceptions when pickle fails (#156)

CHANGELOG.rst

@@ -4,7 +4,7 @@ Changelog
 
 Forthcoming
 -----------
-* ...
+* [blackboards] log a one-shot warning instead of exceptions when pickle fails, `#156 <https://github.com/splintered-reality/py_trees_ros/pull/156>`_
 
 2.0.10 (2020-03-06)
 -------------------

py_trees_ros/blackboard.py

@@ -43,13 +43,19 @@
 class SubBlackboard(object):
     """
     Dynamically track the entire blackboard or part thereof and
-    flag when there have been changes. Not really happy with this class
-    (pickle problems) nor it's name (misleading).
+    flag when there have been changes. This is a nice to have that only
+    works if the blackboard contains fundamental variables (i.e. objects
+    that can be pickled).
+
+    Args:
+        node: the node handle for ros logging of warnings if needed
     """
-    def __init__(self):
+    def __init__(self, node: rclpy.node.Node):
         self.is_changed = False
         self.variable_names = set()
         self.pickled_storage = None
+        self.node = node
+        self.warned = False
 
     def update(self, variable_names: typing.Set[str]):
         """
@@ -60,14 +66,21 @@ def update(self, variable_names: typing.Set[str]):
         Args:
             variable_names: constrain the scope to track for changes
         """
-        # TODO: can we pickle without doing a copy?
-        # TODO: can we use __repr__ as a means of not being prone to pickle
-        #       i.e. put the work back on the user
-        # TODO: catch exceptions thrown by bad pickles
-        # TODO: use a better structure in the blackboard (e.g. JSON) so
-        #       that this isn't brittle w.r.t. pickle failures
+        def handle_pickling_error():
+            if not self.warned:
+                self.node.get_logger().warning("You have objects on the blackboard that can't be pickled.")
+                self.node.get_logger().warning("Any blackboard watchers will always receive updates,")
+                self.node.get_logger().warning("regardless of whether the data changed or not")
+                self.warned = True
+            self.pickled_storage = None
+            self.is_changed = True
+
         if variable_names is None:
-            storage = copy.deepcopy(py_trees.blackboard.Blackboard.storage)
+            try:
+                storage = copy.deepcopy(py_trees.blackboard.Blackboard.storage)
+            except (TypeError, pickle.PicklingError):
+                handle_pickling_error()
+                return
             self.variable_names = py_trees.blackboard.Blackboard.keys()
         else:
             storage = {}
@@ -77,10 +90,17 @@ def update(self, variable_names: typing.Set[str]):
                         py_trees.blackboard.Blackboard.get(variable_name)
                     )
                 except KeyError:
-                    pass  # silently just ignore the request
+                    pass  # not an error, just not on the blackboard yet
+                except (TypeError, pickle.PicklingError):
+                    handle_pickling_error()
+                    return
             self.variable_names = variable_names
-        pickled_storage = pickle.dumps(storage, -1)
-        self.is_changed = pickled_storage != self.pickled_storage
+        try:
+            pickled_storage = pickle.dumps(storage, -1)
+            self.is_changed = pickled_storage != self.pickled_storage
+        except (TypeError, pickle.PicklingError):
+            handle_pickling_error()
+            return
         self.pickled_storage = pickled_storage
 
     def __str__(self):
@@ -131,7 +151,7 @@ def __init__(
     ):
         self.topic_name = topic_name
         self.variable_names = variable_names
-        self.sub_blackboard = SubBlackboard()
+        self.sub_blackboard = SubBlackboard(node=node)
         self.sub_activity_stream = py_trees.blackboard.ActivityStream()
         self.node = node
         self.publisher = self.node.create_publisher(

tests/test_blackboard.py

@@ -0,0 +1,62 @@
+#!/usr/bin/env python3
+# -*- coding: utf-8 -*-
+#
+# License: BSD
+#   https://raw.githubusercontent.com/splintered-reality/py_trees/devel/LICENSE
+#
+
+##############################################################################
+# Imports
+##############################################################################
+
+import threading
+
+import py_trees
+import py_trees.console as console
+import py_trees_ros
+import rclpy
+
+##############################################################################
+# Helpers
+##############################################################################
+
+
+def assert_banner():
+    print(console.green + "----- Asserts -----" + console.reset)
+
+
+def assert_details(text, expected, result):
+    print(console.green + text +
+          "." * (40 - len(text)) +
+          console.cyan + "{}".format(expected) +
+          console.yellow + " [{}]".format(result) +
+          console.reset)
+
+
+def setup_module(module):
+    console.banner("ROS Init")
+    rclpy.init()
+
+
+def teardown_module(module):
+    console.banner("ROS Shutdown")
+    rclpy.shutdown()
+
+##############################################################################
+# Tests
+##############################################################################
+
+
+def test_blackboard_pickle():
+    console.banner("Test Pickle Failure")
+    node = rclpy.create_node("pickle")
+    print("Set 'foo' to a thread lock object")
+    py_trees.blackboard.Blackboard.set("foo", threading.Lock())
+    print("Create Sub Blackboard")
+    sub_blackboard = py_trees_ros.blackboard.SubBlackboard(node=node)
+    print("Update with warning - will raise exceptions if pickle errors are not caught")
+    sub_blackboard.update({"foo"})
+    assert(True)
+    print("Update Quietly")
+    sub_blackboard.update({"foo"})
+    assert(True)