Kill leftovers of pre-MVCC read conflicts

In the early days, before MVCC was introduced, ZODB used to raise
ReadConflictError on access to object that was simultaneously changed by
another client in concurrent transaction. However, as
doc/articles/ZODB-overview.rst says

	Since Zope 2.8 ZODB has implemented **Multi Version Concurrency Control**.
	This means no more ReadConflictErrors, each transaction is guaranteed to be
	able to load any object as it was when the transaction begun.

So today the only way to get a ReadConflictError should be at commit time
for an object that was requested to stay unchanged via
checkCurrentSerialInTransaction.

However MVCCAdapterInstance.load(), instead of reporting "no data", was
still raising ReadConflictError for a deleted or not-yet-created object.
If an object is deleted and later requested to be loaded, it should be
"key not found in database", i.e. POSKeyError, not ReadConflictError.
Fix it.

Adjust docstring of ReadConflictError accordingly to explicitly describe
that this error can only happen at commit time for objects requested to
be current.

There were also leftover code, comment and test bits in Connection,
interfaces, transact, testmvcc and testZODB, that are corrected/removed
correspondingly. testZODB actually had ReadConflictTests that was
completely deactivated: commit b0f992f ("Removed the mvcc option..."; 2007)
moved read-conflict-on-access related tests out of ZODBTests, but did not
activated moved parts at all, because as that commit says when MVCC is
always on unconditionally, there is no on-access conflicts:

    Removed the mvcc option.  Everybody wants mvcc and removing us lets us
    simplify the code a little. (We'll be able to simplify more when we
    stop supporting versions.)

Today, if I try to manually activate that ReadConflictTests via

    @@ -637,6 +637,7 @@ def __init__(self, poisonedjar):
     def test_suite():
         return unittest.TestSuite((
             unittest.makeSuite(ZODBTests, 'check'),
    +        unittest.makeSuite(ReadConflictTests, 'check'),
             ))

     if __name__ == "__main__":

it fails in dumb way showing that this tests were unmaintained for ages:

    Error in test checkReadConflict (ZODB.tests.testZODB.ReadConflictTests)
    Traceback (most recent call last):
      File "/usr/lib/python2.7/unittest/case.py", line 320, in run
        self.setUp()
      File "/home/kirr/src/wendelin/z/ZODB/src/ZODB/tests/testZODB.py", line 451, in setUp
        ZODB.tests.utils.TestCase.setUp(self)
    AttributeError: 'module' object has no attribute 'utils'

Since today ZODB always uses MVCC and there is no way to get
ReadConflictError on access, those tests should be also gone together
with old pre-MVCC way of handling concurrency.

/cc @jimfulton

src/ZODB/Connection.py

@@ -148,10 +148,8 @@ def before_instance(before):
         self._pre_cache = {}
 
         # List of all objects (not oids) registered as modified by the
-        # persistence machinery, or by add(), or whose access caused a
-        # ReadConflictError (just to be able to clean them up from the
-        # cache on abort with the other modified objects). All objects
-        # of this list are either in _cache or in _added.
+        # persistence machinery.
+        # All objects of this list are either in _cache or in _added.
         self._registered_objects = [] # [object]
 
         # ids and serials of objects for which readCurrent was called
@@ -182,15 +180,6 @@ def before_instance(before):
         # in the cache on abort and in other connections on finish.
         self._modified = [] # [oid]
 
-        # We intend to prevent committing a transaction in which
-        # ReadConflictError occurs.  _conflicts is the set of oids that
-        # experienced ReadConflictError.  Any time we raise ReadConflictError,
-        # the oid should be added to this set, and we should be sure that the
-        # object is registered.  Because it's registered, Connection.commit()
-        # will raise ReadConflictError again (because the oid is in
-        # _conflicts).
-        self._conflicts = {}
-
         # To support importFile(), implemented in the ExportImport base
         # class, we need to run _importDuringCommit() from our commit()
         # method.  If _import is not None, it is a two-tuple of arguments
@@ -460,7 +449,6 @@ def _abort(self):
 
     def _tpc_cleanup(self):
         """Performs cleanup operations to support tpc_finish and tpc_abort."""
-        self._conflicts.clear()
         self._needs_to_join = True
         self._registered_objects = []
         self._creating.clear()
@@ -528,8 +516,6 @@ def _commit(self, transaction):
         for obj in self._registered_objects:
             oid = obj._p_oid
             assert oid
-            if oid in self._conflicts:
-                raise ReadConflictError(object=obj)
 
             if obj._p_jar is not self:
                 raise InvalidObjectReference(obj, obj._p_jar)

src/ZODB/POSException.py

@@ -144,10 +144,11 @@ def get_serials(self):
         return self.serials
 
 class ReadConflictError(ConflictError):
-    """Conflict detected when object was loaded.
+    """Conflict detected when object was requested to stay unchanged.
 
-    An attempt was made to read an object that has changed in another
-    transaction (eg. another thread or process).
+    An object was requested to stay not modified via
+    checkCurrentSerialInTransaction, and at commit time was found to be
+    changed by another transaction (eg. another thread or process).
     """
     def __init__(self, message=None, object=None, serials=None, **kw):
         if message is None:

src/ZODB/interfaces.py

@@ -78,12 +78,6 @@ class IConnection(Interface):
     Two options affect consistency.  By default, the mvcc and synch
     options are enabled by default.
 
-    If you pass mvcc=False to db.open(), the Connection will never read
-    non-current revisions of an object.  Instead it will raise a
-    ReadConflictError to indicate that the current revision is
-    unavailable because it was written after the current transaction
-    began.
-
     The logic for handling modifications assumes that the thread that
     opened a Connection (called db.open()) is the thread that will use
     the Connection.  If this is not true, you should pass synch=False

src/ZODB/mvccadapter.py

@@ -152,7 +152,7 @@ def load(self, oid):
         assert self._start is not None
         r = self._storage.loadBefore(oid, self._start)
         if r is None:
-            raise POSException.ReadConflictError(repr(oid))
+            raise POSException.POSKeyError(oid)
         return r[:2]
 
     def prefetch(self, oids):

src/ZODB/tests/testZODB.py

@@ -13,7 +13,6 @@
 ##############################################################################
 from persistent import Persistent
 from persistent.mapping import PersistentMapping
-from ZODB.POSException import ReadConflictError
 from ZODB.POSException import TransactionFailedError
 
 import doctest
@@ -445,156 +444,6 @@ def checkMultipleUndoInOneTransaction(self):
             transaction.abort()
             conn.close()
 
-class ReadConflictTests(ZODB.tests.util.TestCase):
-
-    def setUp(self):
-        ZODB.tests.utils.TestCase.setUp(self)
-        self._storage = ZODB.MappingStorage.MappingStorage()
-
-    def readConflict(self, shouldFail=True):
-        # Two transactions run concurrently.  Each reads some object,
-        # then one commits and the other tries to read an object
-        # modified by the first.  This read should fail with a conflict
-        # error because the object state read is not necessarily
-        # consistent with the objects read earlier in the transaction.
-
-        tm1 = transaction.TransactionManager()
-        conn = self._db.open(transaction_manager=tm1)
-        r1 = conn.root()
-        r1["p"] = self.obj
-        self.obj.child1 = P()
-        tm1.get().commit()
-
-        # start a new transaction with a new connection
-        tm2 = transaction.TransactionManager()
-        cn2 = self._db.open(transaction_manager=tm2)
-        # start a new transaction with the other connection
-        r2 = cn2.root()
-
-        self.assertEqual(r1._p_serial, r2._p_serial)
-
-        self.obj.child2 = P()
-        tm1.get().commit()
-
-        # resume the transaction using cn2
-        obj = r2["p"]
-        # An attempt to access obj should fail, because r2 was read
-        # earlier in the transaction and obj was modified by the othe
-        # transaction.
-        if shouldFail:
-            self.assertRaises(ReadConflictError, lambda: obj.child1)
-            # And since ReadConflictError was raised, attempting to commit
-            # the transaction should re-raise it.  checkNotIndependent()
-            # failed this part of the test for a long time.
-            self.assertRaises(ReadConflictError, tm2.get().commit)
-
-            # And since that commit failed, trying to commit again should
-            # fail again.
-            self.assertRaises(TransactionFailedError, tm2.get().commit)
-            # And again.
-            self.assertRaises(TransactionFailedError, tm2.get().commit)
-            # Etc.
-            self.assertRaises(TransactionFailedError, tm2.get().commit)
-
-        else:
-            # make sure that accessing the object succeeds
-            obj.child1
-        tm2.get().abort()
-
-
-    def checkReadConflict(self):
-        self.obj = P()
-        self.readConflict()
-
-    def checkReadConflictIgnored(self):
-        # Test that an application that catches a read conflict and
-        # continues can not commit the transaction later.
-        root = self._db.open().root()
-        root["real_data"] = real_data = PersistentMapping()
-        root["index"] = index = PersistentMapping()
-
-        real_data["a"] = PersistentMapping({"indexed_value": 0})
-        real_data["b"] = PersistentMapping({"indexed_value": 1})
-        index[1] = PersistentMapping({"b": 1})
-        index[0] = PersistentMapping({"a": 1})
-        transaction.commit()
-
-        # load some objects from one connection
-        tm = transaction.TransactionManager()
-        cn2 = self._db.open(transaction_manager=tm)
-        r2 = cn2.root()
-        real_data2 = r2["real_data"]
-        index2 = r2["index"]
-
-        real_data["b"]["indexed_value"] = 0
-        del index[1]["b"]
-        index[0]["b"] = 1
-        transaction.commit()
-
-        del real_data2["a"]
-        try:
-            del index2[0]["a"]
-        except ReadConflictError:
-            # This is the crux of the text.  Ignore the error.
-            pass
-        else:
-            self.fail("No conflict occurred")
-
-        # real_data2 still ready to commit
-        self.assertTrue(real_data2._p_changed)
-
-        # index2 values not ready to commit
-        self.assertTrue(not index2._p_changed)
-        self.assertTrue(not index2[0]._p_changed)
-        self.assertTrue(not index2[1]._p_changed)
-
-        self.assertRaises(ReadConflictError, tm.get().commit)
-        self.assertRaises(TransactionFailedError, tm.get().commit)
-        tm.get().abort()
-
-    def checkReadConflictErrorClearedDuringAbort(self):
-        # When a transaction is aborted, the "memory" of which
-        # objects were the cause of a ReadConflictError during
-        # that transaction should be cleared.
-        root = self._db.open().root()
-        data = PersistentMapping({'d': 1})
-        root["data"] = data
-        transaction.commit()
-
-        # Provoke a ReadConflictError.
-        tm2 = transaction.TransactionManager()
-        cn2 = self._db.open(transaction_manager=tm2)
-        r2 = cn2.root()
-        data2 = r2["data"]
-
-        data['d'] = 2
-        transaction.commit()
-
-        try:
-            data2['d'] = 3
-        except ReadConflictError:
-            pass
-        else:
-            self.fail("No conflict occurred")
-
-        # Explicitly abort cn2's transaction.
-        tm2.get().abort()
-
-        # cn2 should retain no memory of the read conflict after an abort(),
-        # but 3.2.3 had a bug wherein it did.
-        data_conflicts = data._p_jar._conflicts
-        data2_conflicts = data2._p_jar._conflicts
-        self.assertFalse(data_conflicts)
-        self.assertFalse(data2_conflicts)  # this used to fail
-
-        # And because of that, we still couldn't commit a change to data2['d']
-        # in the new transaction.
-        cn2.sync()  # process the invalidation for data2['d']
-        data2['d'] = 3
-        tm2.get().commit()  # 3.2.3 used to raise ReadConflictError
-
-        cn2.close()
-
 class PoisonedError(Exception):
     pass
 

src/ZODB/tests/testmvcc.py

@@ -25,9 +25,8 @@
 always see a consistent view of the database while it is executing.
 If transaction A is running, has already read an object O1, and a
 different transaction B modifies object O2, then transaction A can no
-longer read the current revision of O2.  It must either read the
-version of O2 that is consistent with O1 or raise a ReadConflictError.
-When MVCC is in use, A will do the former.
+longer read the current revision of O2.  It must read the
+version of O2 that is consistent with O1.
 
 This note includes doctests that explain how MVCC is implemented (and
 test that the implementation is correct).  The tests use a
@@ -387,7 +386,7 @@
 
 We'll reuse the code from the example above, except that there will
 only be a single revision of "b."  As a result, the attempt to
-activate "b" will result in a ReadConflictError.
+activate "b" will result in a POSKeyError.
 
 >>> ts = TestStorage()
 >>> db = DB(ts)
@@ -414,7 +413,7 @@
 >>> r1["b"]._p_activate() # doctest: +ELLIPSIS
 Traceback (most recent call last):
  ...
-ReadConflictError: ...
+POSKeyError: ...
 
 >>> db.close()
 """
@@ -428,7 +427,7 @@
     (re.compile("b('.*?')"), r"\1"),
     # Python 3 adds module name to exceptions.
     (re.compile("ZODB.POSException.ConflictError"), r"ConflictError"),
-    (re.compile("ZODB.POSException.ReadConflictError"), r"ReadConflictError"),
+    (re.compile("ZODB.POSException.POSKeyError"), r"POSKeyError"),
     ])
 
 def test_suite():

src/ZODB/transact.py

@@ -13,7 +13,7 @@
 ##############################################################################
 """Tools to simplify transactions within applications."""
 
-from ZODB.POSException import ReadConflictError, ConflictError
+from ZODB.POSException import ConflictError
 import transaction
 
 def _commit(note):
@@ -40,13 +40,7 @@ def g(*args, **kwargs):
         n = retries
         while n:
             n -= 1
-            try:
-                r = f(*args, **kwargs)
-            except ReadConflictError as msg:
-                transaction.abort()
-                if not n:
-                    raise
-                continue
+            r = f(*args, **kwargs)
             try:
                 _commit(note)
             except ConflictError as msg: