Merge lp:~jamesh/storm/zstorm-same-thread into lp:storm

[James Henstridge]

[James Henstridge (jamesh)]

Before joining a store to the transaction, make sure that ZStorm knows about the store.

The most common case where this check fails is if a ZStorm managed store is being used in a thread other than the one it was created in, which is a semi-common error that can be difficult to debug.

[Thomas Herve (therve)]

Looks great. Just one comment:

[1]
133 + except ZStormError:
134 + # Expected: wrong thread.
135 + pass

To make the test more robust, can you add something to failures here, and assert it's present at the end? This way it will still check something even if the thread doesn't run, for example.

review: Approve

[lp:~jamesh/storm/zstorm-same-thread]

359. By James Henstridge on 2010-02-15

Improve test robustness, as suggested by therve.

[Jamu Kakar (jkakar)]

[1]

+def register_store_with_transaction(store, zstorm_ref):

Can you please add a docstring to this function.

[2]

+ # Check if the store is known. This could indicate a store being

s/is known/is know/ please

[3]

+ """If a zstorm registered thread crosses over to another thread,
+ it will not be usable."""

Please format this as:

        """
        If a zstorm registered thread crosses over to another thread,
        it will not be usable.
        """

[4]

+ try:
+ store.execute("SELECT 1")
+ # And again, to show that the event handler is still
+ # attached.
+ store.execute("SELECT 1")
+ except ZStormError:

Without thinking hard, it's not obvious which of these statements
causes the ZStormError to be raised. I think it would be helpful to
add something to the failures list after each of these calls to make
the flow of execution more clear.

This is nice, debugging the kinds of problems this logic will
uncover is rather awkward. +1!

review: Approve

[James Henstridge (jamesh)]

> [4]
>
> + try:
> + store.execute("SELECT 1")
> + # And again, to show that the event handler is still
> + # attached.
> + store.execute("SELECT 1")
> + except ZStormError:
>
> Without thinking hard, it's not obvious which of these statements
> causes the ZStormError to be raised. I think it would be helpful to
> add something to the failures list after each of these calls to make
> the flow of execution more clear.

Looking at the test, this part is obviously wrong. The second statement never gets run. I'll fix this before merging.

[lp:~jamesh/storm/zstorm-same-thread]

360. By James Henstridge on 2010-07-01

Fix test to properly check repeated calls

361. By James Henstridge on 2010-07-01

Merge from trunk

362. By James Henstridge on 2010-07-01

Add news item

=== modified file 'NEWS'
--- NEWS	2010-06-04 15:23:13 +0000
+++ NEWS	2010-07-01 17:53:23 +0000
@@ -11,6 +11,9 @@
    unblock_access() methods that can be used to block access to the
    database connection.  This can be used to ensure that an
    application doesn't access the database at unexpected times.
+ - When using stores managed by ZStorm, a ZStormError will be raised
+   on attempts to use a per-thread store from the wrong thread (bug
+   #348815).
 
 Bug fixes
 ---------

=== modified file 'storm/zope/zstorm.py'
--- storm/zope/zstorm.py	2009-06-26 08:59:26 +0000
+++ storm/zope/zstorm.py	2010-07-01 17:53:23 +0000
@@ -134,7 +134,7 @@
         store._register_for_txn = True
         store._event.hook(
             "register-transaction", register_store_with_transaction,
-            self.transaction_manager)
+            weakref.ref(self))
 
         self._stores[id(store)] = store
         if name is not None:
@@ -180,7 +180,7 @@
         store._register_for_txn = False
         store._event.unhook(
             "register-transaction", register_store_with_transaction,
-            self.transaction_manager)
+            weakref.ref(self))
 
     def iterstores(self):
         """Iterate C{name, store} 2-tuples."""
@@ -203,9 +203,20 @@
         return self._default_uris.copy()
 
 
-def register_store_with_transaction(store, transaction_manager):
-    data_manager = StoreDataManager(store, transaction_manager)
-    transaction_manager.get().join(data_manager)
+def register_store_with_transaction(store, zstorm_ref):
+    zstorm = zstorm_ref()
+    if zstorm is None:
+        # zstorm object does not exist any more.
+        return False
+
+    # Check if the store is  known.  This could indicate a store being
+    # used outside of its thread.
+    if id(store) not in zstorm._stores:
+        raise ZStormError("Store not registered with ZStorm, or registered "
+                          "with another thread.")
+
+    data_manager = StoreDataManager(store, zstorm)
+    zstorm.transaction_manager.get().join(data_manager)
 
     # Unhook the event handler.  It will be rehooked for the next transaction.
     return False
@@ -216,9 +227,10 @@
 
     implements(IDataManager)
 
-    def __init__(self, store, transaction_manager):
+    def __init__(self, store, zstorm):
         self._store = store
-        self.transaction_manager = transaction_manager
+        self._zstorm = zstorm
+        self.transaction_manager = zstorm.transaction_manager
 
     def abort(self, txn):
         try:
@@ -227,7 +239,7 @@
             if self._store._register_for_txn:
                 self._store._event.hook(
                     "register-transaction", register_store_with_transaction,
-                    self.transaction_manager)
+                    weakref.ref(self._zstorm))
 
     def tpc_begin(self, txn):
         # Zope's transaction system will call tpc_begin() on all
@@ -245,7 +257,7 @@
             if self._store._register_for_txn:
                 self._store._event.hook(
                     "register-transaction", register_store_with_transaction,
-                    self.transaction_manager)
+                    weakref.ref(self._zstorm))
 
     def tpc_vote(self, txn):
         pass

=== modified file 'tests/zope/zstorm.py'
--- tests/zope/zstorm.py	2009-03-05 21:53:12 +0000
+++ tests/zope/zstorm.py	2010-07-01 17:53:23 +0000
@@ -18,7 +18,9 @@
 # You should have received a copy of the GNU Lesser General Public License
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 #
-import thread, weakref, gc
+import threading
+import weakref
+import gc
 
 from tests.helper import TestHelper
 from tests.zope import has_transaction, has_zope_component
@@ -77,18 +79,14 @@
 
         raised = []
 
-        lock = thread.allocate_lock()
-        lock.acquire()
         def f():
             try:
-                try:
-                    self.zstorm.get("name")
-                except ZStormError:
-                    raised.append(True)
-            finally:
-                lock.release()
-        thread.start_new_thread(f, ())
-        lock.acquire()
+                self.zstorm.get("name")
+            except ZStormError:
+                raised.append(True)
+        thread = threading.Thread(target=f)
+        thread.start()
+        thread.join()
 
         self.assertTrue(raised)
 
@@ -218,6 +216,31 @@
         store.execute("SELECT 1")
         self.assertNotInTransaction(store)
 
+    def test_wb_cross_thread_store_does_not_join_transaction(self):
+        """If a zstorm registered thread crosses over to another thread,
+        it will not be usable."""
+        store = self.zstorm.get("name", "sqlite:")
+
+        failures = []
+        def f():
+            # We perform this twice to show that ZStormError is raised
+            # consistently (i.e. not just the first time).
+            for i in range(2):
+                try:
+                    store.execute("SELECT 1")
+                except ZStormError:
+                    failures.append("ZStormError raised")
+                except Exception, exc:
+                    failures.append("Expected ZStormError, got %r" % exc)
+                else:
+                    failures.append("Expected ZStormError, nothing raised")
+                if self._isInTransaction(store):
+                    failures.append("store was joined to transaction")
+        thread = threading.Thread(target=f)
+        thread.start()
+        thread.join()
+        self.assertEqual(failures, ["ZStormError raised"] * 2)
+
     def test_wb_reset(self):
         """_reset is used to reset the zstorm utility between zope test runs.
         """