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

@@ -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',

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

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