[Checkins] SVN: zope.sqlalchemy/trunk/ Remove the session id from the SESSION_STATE just before we de-reference the

[Brian Sutherland]

Log message for revision 91205:
  Remove the session id from the SESSION_STATE just before we de-reference the
  session (i.e. all work is already successfuly completed). This fixes cases
  where the transaction commit failed but SESSION_STATE was already cleared.  In
  those cases, the transaction was wedeged as abort would always error.  This
  happened on PostgreSQL where invalid SQL was used and the error caught.
  

Changed:
  U   zope.sqlalchemy/trunk/CHANGES.txt
  U   zope.sqlalchemy/trunk/src/zope/sqlalchemy/datamanager.py
  U   zope.sqlalchemy/trunk/src/zope/sqlalchemy/tests.py

-=-
Modified: zope.sqlalchemy/trunk/CHANGES.txt
===================================================================
--- zope.sqlalchemy/trunk/CHANGES.txt	2008-09-17 06:41:09 UTC (rev 91204)
+++ zope.sqlalchemy/trunk/CHANGES.txt	2008-09-17 10:31:54 UTC (rev 91205)
@@ -7,6 +7,11 @@
 Bugs fixed:
 
 * Only raise errors in tpc_abort if we have committed.
+* Remove the session id from the SESSION_STATE just before we de-reference the
+  session (i.e. all work is already successfuly completed). This fixes cases
+  where the transaction commit failed but SESSION_STATE was already cleared.  In
+  those cases, the transaction was wedeged as abort would always error.  This
+  happened on PostgreSQL where invalid SQL was used and the error caught.
 
 0.3 (2008-07-29)
 ----------------

Modified: zope.sqlalchemy/trunk/src/zope/sqlalchemy/datamanager.py
===================================================================
--- zope.sqlalchemy/trunk/src/zope/sqlalchemy/datamanager.py	2008-09-17 06:41:09 UTC (rev 91204)
+++ zope.sqlalchemy/trunk/src/zope/sqlalchemy/datamanager.py	2008-09-17 10:31:54 UTC (rev 91205)
@@ -49,31 +49,32 @@
         _SESSION_STATE[id(session)] = status
         self.state = 'init'
 
+    def _finish(self, final_state):
+        assert self.session is not None
+        del _SESSION_STATE[id(self.session)]
+        self.tx = self.session = None
+        self.state = final_state
+
     def abort(self, trans):
         if self.tx is not None: # there may have been no work to do
-            del _SESSION_STATE[id(self.session)]
             self.session.close()
-            self.tx = self.session = None
-            self.state = 'aborted'
+            self._finish('aborted')
 
     def tpc_begin(self, trans):
         self.session._autoflush()
     
     def commit(self, trans):
         status = _SESSION_STATE[id(self.session)]
-        del _SESSION_STATE[id(self.session)]
         if status is not STATUS_INVALIDATED:
             self.session.close()
-            self.tx = self.session = None
-            self.state = 'no work'
+            self._finish('no work')
 
     def tpc_vote(self, trans):
         # for a one phase data manager commit last in tpc_vote
         if self.tx is not None: # there may have been no work to do
             self.tx.commit()
             self.session.close()
-            self.tx = self.session = None
-            self.state = 'committed'
+            self._finish('committed')
                 
     def tpc_finish(self, trans):
         pass
@@ -114,15 +115,13 @@
         if self.tx is not None:
             self.tx.commit()
             self.session.close()
-            self.tx = self.session = None
-            self.state = 'committed'
+            self._finish('committed')
 
     def tpc_abort(self, trans):
         if self.tx is not None: # we may not have voted, and been aborted already
             self.tx.rollback()
             self.session.close()
-            self.tx = self.session = None
-            self.state = 'aborted commit'
+            self._finish('aborted commit')
 
     def sortKey(self):
         # Sort normally

Modified: zope.sqlalchemy/trunk/src/zope/sqlalchemy/tests.py
===================================================================
--- zope.sqlalchemy/trunk/src/zope/sqlalchemy/tests.py	2008-09-17 06:41:09 UTC (rev 91204)
+++ zope.sqlalchemy/trunk/src/zope/sqlalchemy/tests.py	2008-09-17 10:31:54 UTC (rev 91205)
@@ -32,6 +32,7 @@
 import sqlalchemy as sa
 from sqlalchemy import orm, sql
 from zope.sqlalchemy import datamanager as tx
+from zope.sqlalchemy import mark_changed
 
 TEST_TWOPHASE = bool(os.environ.get('TEST_TWOPHASE'))
 TEST_DSN = os.environ.get('TEST_DSN', 'sqlite:///:memory:')
@@ -169,6 +170,32 @@
         transaction.abort()
         metadata.drop_all(engine)
 
+    def testAbortAfterCommit(self):
+        # This is a regression test which used to wedge the transaction
+        # machinery when using PostgreSQL (and perhaps other) connections.
+        # Basically, if a commit failed, there was no way to abort the
+        # transaction. Leaving the transaction wedged.
+        transaction.begin()
+        session = Session()
+        conn = session.connection()
+        # At least PostgresSQL requires a rollback after invalid SQL is executed
+        self.assertRaises(Exception, conn.execute, "BAD SQL SYNTAX")
+        mark_changed(session)
+        try:
+            # Thus we could fail in commit
+            transaction.commit()
+        except:
+            # But abort must succed (and actually rollback the base connection)
+            transaction.abort()
+            pass
+        # Or the next transaction the next transaction will not be able to start!
+        transaction.begin()
+        session = Session()
+        conn = session.connection()
+        conn.execute("SELECT 1")
+        mark_changed(session)
+        transaction.commit()
+
     def testSimplePopulation(self):
         session = Session()
         query = session.query(User)