Change handling of tpc_abort() during disconnection.

Fix likely bug where calling tpc_abort() after a client disconnected
did not properly clear the client's internal state about the
transaction.  The change means that tpc_abort() will never raise a
ClientDisconnected error, even when disconnected.

Added a new test that sort-of covers this case and deleted another
that was testing, in part, that you did get a ClientDisconnected
error.

src/ZEO/ClientStorage.py

@@ -758,12 +758,20 @@ class ClientStorage:
         """Storage API: abort a transaction."""
         if transaction is not self._transaction:
             return
+        try:
+            # XXX Are there any transactions that should prevent an
+            # abort from occurring?  It seems wrong to swallow them
+            # all, yet you want to be sure that other abort logic is
+            # executed regardless.
             try:
                 self._server.tpc_abort(self._serial)
+            except ClientDisconnected:
+                # log the error and continue
+                pass
+        finally:
             self._tbuf.clear()
             self._seriald.clear()
             del self._serials[:]
-        finally:
             self.end_transaction()
 
     def tpc_finish(self, transaction, f=None):

src/ZEO/tests/ConnectionTests.py

@@ -282,6 +282,41 @@ class ConnectionTests(CommonSetupTearDown):
         self.assertRaises(ClientDisconnected,
                           self._storage.load, 'fredwash', '')
 
+    def checkDisconnectedAbort(self):
+        self._storage = self.openClientStorage()
+        self._dostore()
+        oids = [self._storage.new_oid() for i in range(5)]
+        txn = Transaction()
+        self._storage.tpc_begin(txn)
+        for oid in oids:
+            data = zodb_pickle(MinPO(oid))
+            self._storage.store(oid, None, data, '', txn)
+        self.shutdownServer()
+        self.assertRaises(ClientDisconnected, self._storage.tpc_vote, txn)
+        self._storage.tpc_abort(txn)
+        self.startServer(create=0)
+        self._storage._wait()
+        self._dostore()
+
+        # This test is supposed to cover the following error, although
+        # I don't have much confidence that it does.  The likely
+        # explanation for the error is that the _tbuf contained
+        # objects that weren't in the _seriald, because the client was
+        # interrupted waiting for tpc_vote() to return.  When the next
+        # transaction committed, it tried to do something with the
+        # bogus _tbuf entries.  The exaplanation is wrong/incomplete,
+        # because tpc_begin() should clear the _tbuf.
+
+        # 2003-01-15T15:44:19 ERROR(200) ZODB A storage error occurred
+        # in the last phase of a two-phase commit.  This shouldn't happen.
+
+        # Traceback (innermost last):
+        # Module ZODB.Transaction, line 359, in _finish_one
+        # Module ZODB.Connection, line 691, in tpc_finish
+        # Module ZEO.ClientStorage, line 679, in tpc_finish
+        # Module ZEO.ClientStorage, line 709, in _update_cache
+        # KeyError: ...
+
     def checkBasicPersistence(self):
         # Verify cached data persists across client storage instances.
 

src/ZEO/tests/ThreadTests.py

@@ -73,23 +73,6 @@ class GetsThroughBeginThread(BasicThread):
             self.gotValueError = 1
 
 
-class AbortsAfterBeginFailsThread(BasicThread):
-    # This class is identical to GetsThroughBeginThread except that it
-    # attempts to tpc_abort() after the tpc_begin() fails.  That will raise a
-    # ClientDisconnected exception which implies that we don't have the lock,
-    # and that's what we really want to test (but it's difficult given the
-    # threading module's API).
-    def run(self):
-        try:
-            self.storage.tpc_begin(self.trans)
-        except ZEO.ClientStorage.ClientStorageError:
-            self.gotValueError = 1
-        try:
-            self.storage.tpc_abort(self.trans)
-        except ClientDisconnected:
-            self.gotDisconnected = 1
-
-
 class ThreadTests:
     # Thread 1 should start a transaction, but not get all the way through it.
     # Main thread should close the connection.  Thread 1 should then get
@@ -128,24 +111,6 @@ class ThreadTests:
         self.assertEqual(thread1.gotValueError, 1)
         self.assertEqual(thread2.gotValueError, 1)
 
-    def checkThatFailedBeginDoesNotHaveLock(self):
-        doNextEvent = threading.Event()
-        threadStartedEvent = threading.Event()
-        thread1 = GetsThroughVoteThread(self._storage,
-                                        doNextEvent, threadStartedEvent)
-        thread2 = AbortsAfterBeginFailsThread(self._storage,
-                                              doNextEvent, threadStartedEvent)
-        thread1.start()
-        threadStartedEvent.wait(1)
-        thread2.start()
-        self._storage.close()
-        doNextEvent.set()
-        thread1.join()
-        thread2.join()
-        self.assertEqual(thread1.gotValueError, 1)
-        self.assertEqual(thread2.gotValueError, 1)
-        self.assertEqual(thread2.gotDisconnected, 1)
-
     # Run a bunch of threads doing small and large stores in parallel
     def checkMTStores(self):
         threads = []