Repository: meejah/txtorcon

Issue:
What happens to an exception in event listener?
If an event-handler for an event listener causes an exception, where does it end up going?


Discussion:

Patch:
diff --git a/txtorcon/router.py b/txtorcon/router.py
index 547c321..03b6163 100644
--- a/txtorcon/router.py
+++ b/txtorcon/router.py
@@ -200,8 +200,6 @@ class Router(object):
             )
         returnValue(relay_data)
 
-
-
     # note that exit-policy is no longer included in the
     # microdescriptors by default, so this stuff is mostly here as a
     # historic artifact. If you want to use exit-policy for things
diff --git a/txtorcon/torcontrolprotocol.py b/txtorcon/torcontrolprotocol.py
index 269c72e..9947c94 100644
--- a/txtorcon/torcontrolprotocol.py
+++ b/txtorcon/torcontrolprotocol.py
@@ -11,6 +11,7 @@ from twisted.internet import defer
 from twisted.internet.interfaces import IProtocolFactory
 from twisted.internet.error import ConnectionDone
 from twisted.protocols.basic import LineOnlyReceiver
+from twisted.python.failure import Failure
 
 from zope.interface import implementer
 
@@ -109,7 +110,17 @@ class Event(object):
 
     def got_update(self, data):
         for cb in self.callbacks:
-            cb(data)
+            try:
+                cb(data)
+            except Exception as e:
+                log.err(Failure())
+                log.err(
+                    "Notifying '{callback}' for '{name}' failed: {e}".format(
+                        callback=cb,
+                        name=self.name,
+                        e=e,
+                    )
+                )
 
 
 def unquote(word):


Test patch:
diff --git a/test/test_router.py b/test/test_router.py
index 4ddf5c2..de8dd84 100644
--- a/test/test_router.py
+++ b/test/test_router.py
@@ -3,7 +3,7 @@ from datetime import datetime
 from mock import Mock
 
 from twisted.trial import unittest
-from twisted.internet import defer, error
+from twisted.internet import defer
 from twisted.python.failure import Failure
 from twisted.web.client import ResponseDone
 
@@ -253,7 +253,7 @@ class OnionOOTests(unittest.TestCase):
         agent.request = Mock(return_value=defer.succeed(resp))
 
         with self.assertRaises(Exception) as ctx:
-            data = yield self.router.get_onionoo_details(agent)
+            yield self.router.get_onionoo_details(agent)
 
         self.assertTrue(
             "multiple relays for" in str(ctx.exception)
@@ -279,7 +279,7 @@ class OnionOOTests(unittest.TestCase):
         agent.request = Mock(return_value=defer.succeed(resp))
 
         with self.assertRaises(Exception) as ctx:
-            data = yield self.router.get_onionoo_details(agent)
+            yield self.router.get_onionoo_details(agent)
 
         self.assertTrue(
             " but got data for " in str(ctx.exception)
diff --git a/test/test_torcontrolprotocol.py b/test/test_torcontrolprotocol.py
index a3beda5..03465f0 100644
--- a/test/test_torcontrolprotocol.py
+++ b/test/test_torcontrolprotocol.py
@@ -849,6 +849,40 @@ platform Tor 0.2.5.0-alpha-dev on Linux
         self.send(b"650 STREAM 2345 NEW 4321 2.3.4.5:666 REASON=MISC")
         self.assertEqual(listener.stream_events, 2)
 
+    def test_eventlistener_error(self):
+        self.protocol._set_valid_events('STREAM')
+
+        class EventListener(object):
+            stream_events = 0
+            do_error = False
+
+            def __call__(self, data):
+                self.stream_events += 1
+                if self.do_error:
+                    raise Exception("the bad thing happened")
+
+        # we make sure the first listener has the errors to prove the
+        # second one still gets called.
+        listener0 = EventListener()
+        listener0.do_error = True
+        listener1 = EventListener()
+        self.protocol.add_event_listener('STREAM', listener0)
+        self.protocol.add_event_listener('STREAM', listener1)
+
+        d = self.protocol.defer
+        self.send(b"250 OK")
+        self._wait(d)
+        self.send(b"650 STREAM 1234 NEW 4321 1.2.3.4:555 REASON=MISC")
+        self.send(b"650 STREAM 2345 NEW 4321 2.3.4.5:666 REASON=MISC")
+        self.assertEqual(listener0.stream_events, 2)
+        self.assertEqual(listener1.stream_events, 2)
+
+        # should have logged the two errors
+        logged = self.flushLoggedErrors()
+        self.assertEqual(2, len(logged))
+        self.assertTrue("the bad thing happened" in str(logged[0]))
+        self.assertTrue("the bad thing happened" in str(logged[1]))
+
     def test_remove_eventlistener(self):
         self.protocol._set_valid_events('STREAM')
 
@@ -900,13 +934,12 @@ platform Tor 0.2.5.0-alpha-dev on Linux
         except Exception as e:
             self.assertTrue('FOO' in str(e))
 
-    def checkContinuation(self, v):
-        self.assertEqual(v, "key=\nvalue0\nvalue1")
-
-    def test_continuationLine(self):
+    def test_continuation_line(self):
         d = self.protocol.get_info_raw("key")
 
-        d.addCallback(self.checkContinuation)
+        def check_continuation(v):
+            self.assertEqual(v, "key=\nvalue0\nvalue1")
+        d.addCallback(check_continuation)
 
         self.send(b"250+key=")
         self.send(b"value0")