From 03a326be7fc86aa402fb969883168c55c3776faf Mon Sep 17 00:00:00 2001 From: Jim Fulton Date: Sat, 11 Jun 2016 11:57:51 -0400 Subject: [PATCH] Updated to only call storage synchronizers on explicit transaction begin And in special case where a connectin is opened while a transaction is active (Zope2). --- setup.py | 2 +- src/ZODB/Connection.py | 45 +++++++++++++++++---- src/ZODB/tests/synchronizers.txt | 67 +++++++++++++------------------- 3 files changed, 65 insertions(+), 49 deletions(-) diff --git a/setup.py b/setup.py index bdf8f18da..c7928a48c 100644 --- a/setup.py +++ b/setup.py @@ -141,7 +141,7 @@ def read_file(*path): 'persistent >= 4.2.0', 'BTrees >= 4.2.0', 'ZConfig', - 'transaction >= 1.5.0', + 'transaction >= 1.6.1', 'six', 'zc.lockfile', 'zope.interface', diff --git a/src/ZODB/Connection.py b/src/ZODB/Connection.py index e49b2d5a1..e068a3923 100644 --- a/src/ZODB/Connection.py +++ b/src/ZODB/Connection.py @@ -405,7 +405,7 @@ def _implicitlyAdding(self, oid): def sync(self): """Manually update the view on the database.""" - self.transaction_manager.abort() + self.transaction_manager.begin() def getDebugInfo(self): """Returns a tuple with different items for debugging the @@ -770,10 +770,16 @@ def beforeCompletion(self, txn): # We don't do anything before a commit starts. pass - def newTransaction(self, transaction=None): + def newTransaction(self, transaction, sync=True): self._readCurrent.clear() if self._mvcc_storage: + # XXX It's not clear if we should make calling sync + # dependent on the sync flag here. RelStorage's sync semantics + # are a bit different, plus RelStorage doesn't actually + # work with this code. :/ This will probably be sorted when + # we do: + # https://groups.google.com/forum/#!topic/zodb/QJYmvF0eUUM self._storage.sync(True) # Poll the storage for invalidations. mvc_invalidated = self._storage.poll_invalidations() @@ -782,7 +788,8 @@ def newTransaction(self, transaction=None): # we need to flush the whole cache. self._invalidatedCache = True else: - getattr(self._storage, 'sync', noop)() + if sync: + getattr(self._storage, 'sync', noop)() mvc_invalidated = None if self.opened: @@ -831,7 +838,16 @@ def newTransaction(self, transaction=None): # Now is a good time to collect some garbage. self._cache.incrgc() - afterCompletion = newTransaction + def afterCompletion(self, transaction): + # Note that we we call newTransaction here for 2 reasons: + # a) Applying invalidations early frees up resources + # early. This is especially useful if the connection isn't + # going to be used in a while. + # b) Non-hygienic applications might start new transactions by + # finalizing previous ones without calling begin. We pass + # False to avoid possiblyt expensive sync calls to not + # penalize well-behaved applications that call begin. + self.newTransaction(transaction, False) # Transaction-manager synchronization -- ISynchronizer ########################################################################## @@ -977,15 +993,30 @@ def open(self, transaction_manager=None, delegate=True): self.transaction_manager = transaction_manager - transaction_manager.registerSynch(self) - self.opened = time.time() if self._reset_counter != global_reset_counter: # New code is in place. Start a new cache. self._resetCache() - self.newTransaction() + # This newTransaction is to deal with some pathalogical cases: + # + # a) Someone opens a connection when a transaction isn't + # active and proceeeds without calling begin on a + # transaction manager. We initialize the transaction for + # the connection, but we don't do a storage sync, since + # this will be done if a well-nehaved application calls + # begin, and we don't want to penalize well-behaved + # transactions by syncing twice, as storage syncs might be + # expensive. + # b) Lots of tests assume that connection transaction + # information is set on open. + # + # Fortunately, this is a cheap operation. It doesn't really + # cost much, if anything. + self.newTransaction(None, False) + + transaction_manager.registerSynch(self) if self._cache is not None: self._cache.incrgc() # This is a good time to do some GC diff --git a/src/ZODB/tests/synchronizers.txt b/src/ZODB/tests/synchronizers.txt index 3269b6cc4..501c3968c 100644 --- a/src/ZODB/tests/synchronizers.txt +++ b/src/ZODB/tests/synchronizers.txt @@ -25,27 +25,43 @@ Make a change locally: >>> rt = cn.root() >>> rt['a'] = 1 -Sync is called when a connection is open, as that starts a new transaction: +Sync isn't called when a connectiin is opened, even though that +implicitly starts a new transaction: + + >>> st.sync_called + False + +Sync is only called when we explicitly start a new transaction: + + >>> _ = transaction.begin() >>> st.sync_called True >>> st.sync_called = False +BTW, calling ``sync()`` on a connectin starts a new transaction, which +caused ``sync()`` to be called on the storage: -``sync()`` is called by the Connection's ``afterCompletion()`` hook after the -commit completes. + >>> cn.sync() + >>> st.sync_called + True + >>> st.sync_called = False + +``sync()`` is not called by the Connection's ``afterCompletion()`` +hook after the commit completes, because we'll sunc when a new +transaction begins: >>> transaction.commit() >>> st.sync_called # False before 3.4 - True + False -``sync()`` is also called by the ``afterCompletion()`` hook after an abort. +``sync()`` is also not called by the ``afterCompletion()`` hook after an abort. >>> st.sync_called = False >>> rt['b'] = 2 >>> transaction.abort() >>> st.sync_called # False before 3.4 - True + False And ``sync()`` is called whenever we explicitly start a new transaction, via the ``newTransaction()`` hook. @@ -63,45 +79,14 @@ traceback then ;-) >>> cn.close() -One more, very obscure. It was the case that if the first action a new -threaded transaction manager saw was a ``begin()`` call, then synchronizers -registered after that in the same transaction weren't communicated to the -`Transaction` object, and so the synchronizers' ``afterCompletion()`` hooks -weren't called when the transaction commited. None of the test suites -(ZODB's, Zope 2.8's, or Zope3's) caught that, but apparently Zope 3 takes this -path at some point when serving pages. - - >>> tm = transaction.ThreadTransactionManager() - >>> st.sync_called = False - >>> dummy = tm.begin() # we're doing this _before_ opening a connection - >>> cn = db.open(transaction_manager=tm) - >>> rt = cn.root() # make a change - >>> rt['c'] = 3 - >>> st.sync_called - True - >>> st.sync_called = False - +As a special case, if a synchronizer registers while a transaction is +in flight, then newTransaction and this the storage sync method is +called: -Now ensure that ``cn.afterCompletion() -> st.sync()`` gets called by commit -despite that the `Connection` registered after the transaction began: - - >>> tm.commit() - >>> st.sync_called - True - -And try the same thing with a non-threaded transaction manager: - - >>> cn.close() >>> tm = transaction.TransactionManager() >>> st.sync_called = False - >>> dummy = tm.begin() # we're doing this _before_ opening a connection + >>> _ = tm.begin() # we're doing this _before_ opening a connection >>> cn = db.open(transaction_manager=tm) - >>> rt = cn.root() # make a change - >>> rt['d'] = 4 - >>> st.sync_called - True - >>> st.sync_called = False - >>> tm.commit() >>> st.sync_called True