Merge lp:~diogobaeder/storm/cleanup-timeline into lp:storm

[Sidnei da Silva]

Make sure connection_raw_execute_error is called when the tracer's connection_raw_execute fails, so that the tracers have a chance to clean themselves up.

[James Westby (james-w)]

I've just been discussing this changes with Diogo and Sidnei, and now understand something
I missed first time. This change is most important when using two tracers:

   A -> connection_raw_execute -> success
   B -> connection_raw_execute -> exception

A is then left in a state where it doesn't know that the execution was halted, which
this patch corrects.

Thanks,

James

[Sidnei da Silva (sidnei)]

There's an alternative failure scenario that this fix doesn't cover:

  - If the cursor.execute() raises a DatabaseError, the PostgresTimeoutTracer's connection_raw_execute_error may turn that into a TimeoutError and re-raise.
  - If the TimelineTracer (or any other tracer for that matter) comes last, then it's connection_raw_execute_error will never be called because the exception bubbles up from trace(), never notifying the remaining tracers.

An additional solution to that problem might be to also make the trace() function more robust, ensuring that it always goes through all the tracers and collects all exceptions, raising some sort of MultiException if there is more than one.

The change of behaviour *could* break people relying on the broken behaviour (eg, expecting that if one of the tracers fails, the remaining ones will never execute), but I don't think anyone in their sane mind would do that.

[Free Ekanayaka (free.ekanayaka)]

Looks good, +1!

[1]

+ def test_raw_execute_setup_error_tracing(self):

Please add a docstring for this test. I know it's not there in every old test, but we're trying to do that for new tests.

review: Approve

=== modified file 'storm/database.py'
--- storm/database.py	2012-06-27 18:41:59 +0000
+++ storm/database.py	2013-02-06 22:47:23 +0000
@@ -369,13 +369,21 @@
         @return: The dbapi cursor object, as fetched from L{build_raw_cursor}.
         """
         raw_cursor = self._check_disconnect(self.build_raw_cursor)
-        self._check_disconnect(
-            trace, "connection_raw_execute", self, raw_cursor,
-            statement, params or ())
+        self._prepare_execution(raw_cursor, params, statement)
+        args = self._execution_args(params, statement)
+        self._run_execution(raw_cursor, args, params, statement)
+        return raw_cursor
+
+    def _execution_args(self, params, statement):
+        """Get the appropriate statement execution arguments."""
         if params:
             args = (statement, tuple(self.to_database(params)))
         else:
             args = (statement,)
+        return args
+
+    def _run_execution(self, raw_cursor, args, params, statement):
+        """Complete the statement execution, along with result reports."""
         try:
             self._check_disconnect(raw_cursor.execute, *args)
         except Exception, error:
@@ -387,7 +395,18 @@
             self._check_disconnect(
                 trace, "connection_raw_execute_success", self, raw_cursor,
                 statement, params or ())
-        return raw_cursor
+
+    def _prepare_execution(self, raw_cursor, params, statement):
+        """Prepare the statement execution to be run."""
+        try:
+            self._check_disconnect(
+                trace, "connection_raw_execute", self, raw_cursor,
+                statement, params or ())
+        except Exception, error:
+            self._check_disconnect(
+                trace, "connection_raw_execute_error", self, raw_cursor,
+                statement, params or (), error)
+            raise
 
     def _ensure_connected(self):
         """Ensure that we are connected to the database.
@@ -528,5 +547,5 @@
         factory = module.create_from_uri
     return factory(uri)
 
-# Deal with circular import.        
+# Deal with circular import.
 from storm.tracer import trace

=== modified file 'tests/database.py'
--- tests/database.py	2012-06-27 17:59:50 +0000
+++ tests/database.py	2013-02-06 22:47:23 +0000
@@ -229,6 +229,20 @@
                                         ("ERROR", self.connection, RawCursor,
                                          "something", (), error)])
 
+    def test_raw_execute_setup_error_tracing(self):
+        cursor_mock = self.mocker.patch(FakeTracer)
+        cursor_mock.connection_raw_execute(ARGS)
+        error = ZeroDivisionError()
+        self.mocker.throw(error)
+        self.mocker.replay()
+
+        tracer = FakeTracer()
+        install_tracer(tracer)
+        self.assertRaises(ZeroDivisionError,
+                          self.connection.execute, "something")
+        self.assertEquals(tracer.seen, [("ERROR", self.connection, RawCursor,
+                                         "something", (), error)])
+
     def test_tracing_check_disconnect(self):
         tracer = FakeTracer()
         tracer_mock = self.mocker.patch(tracer)