Merge pull request #174 from zodb/issue173

Workaround TypeError in MySQLOIDAllocator

CHANGES.rst

@@ -17,6 +17,9 @@
 - Oracle: Fix two queries that got broken due to the performance work
   in 2.1a1.
 
+- MySQL: Workaround a rare issue that could lead to a ``TypeError``
+  when getting new OIDs. See :issue:`173`.
+
 2.1a1 (2017-02-01)
 ==================
 

relstorage/adapters/mysql/adapter.py

@@ -116,7 +116,7 @@ class MySQLAdapter(object):
         )
         self.connmanager.add_on_store_opened(self.mover.on_store_opened)
         self.connmanager.add_on_load_opened(self.mover.on_load_opened)
-        self.oidallocator = MySQLOIDAllocator()
+        self.oidallocator = MySQLOIDAllocator(self.connmanager.disconnected_exceptions[0])
         self.txncontrol = MySQLTransactionControl(
             keep_history=self.keep_history,
             Binary=driver.Binary,

relstorage/adapters/mysql/locker.py

@@ -58,6 +58,9 @@ class MySQLLocker(AbstractLocker):
             # This has been observed under certain database drivers and concurrency loads,
             # specifically gevent with umysqldb and high concurrency. It's not clear what the cause
             # is, so lets at least raise a specific message.
+            # It's often TypeError("'NoneType' object has no attribute '__getitem__'",),
+            # meaning that cursor.fetchone() returned nothing. This may be related to
+            # cursor.lastrowid being None in OID allocator sometimes.
             raise CommitLockQueryFailedError("The commit lock query failed: %s" % repr(e))
 
         if nowait and locked in (0, 1):

relstorage/adapters/mysql/oidallocator.py

@@ -26,6 +26,14 @@ from perfmetrics import metricmethod
 @implementer(IOIDAllocator)
 class MySQLOIDAllocator(AbstractOIDAllocator):
 
+    def __init__(self, disconnected_exception):
+        """
+        :param type disconnected_exception: The exception to raise when
+           we get an invalid value for ``lastrowid``.
+        """
+        AbstractOIDAllocator.__init__(self)
+        self.disconnected_exception = disconnected_exception
+
     def set_min_oid(self, cursor, oid):
         """Ensure the next OID is at least the given OID."""
         n = (oid + 15) // 16
@@ -41,6 +49,11 @@ class MySQLOIDAllocator(AbstractOIDAllocator):
         # cursor.connection.insert_id(), which was specific to MySQLdb)
         n = cursor.lastrowid
 
+        # At least in one setup (gevent/umysqldb/pymysql/mysql 5.5)
+        # we have observed cursor.lastrowid to be None.
+        if n is None:
+            raise self.disconnected_exception("Invalid return for lastrowid")
+
         if n % 1000 == 0:
             # Clean out previously generated OIDs.
             stmt = "DELETE FROM new_oid WHERE zoid < %s"

relstorage/tests/testmysql.py

@@ -174,6 +174,18 @@ class HFMySQLToFile(UseMySQLAdapter, HistoryFreeToFileStorage):
 class HFMySQLFromFile(UseMySQLAdapter, HistoryFreeFromFileStorage):
     pass
 
+class TestOIDAllocator(unittest.TestCase):
+
+    def test_bad_rowid(self):
+        from relstorage.adapters.mysql.oidallocator import MySQLOIDAllocator
+        class Cursor(object):
+            def execute(self, s):
+                pass
+            lastrowid = None
+
+        oids = MySQLOIDAllocator(KeyError)
+        self.assertRaises(KeyError, oids.new_oids, Cursor())
+
 db_names = {
     'data': base_dbname,
     '1': base_dbname,
@@ -203,6 +215,7 @@ def test_suite():
 
     suite.addTest(unittest.makeSuite(HPMySQLDestZODBConvertTests))
     suite.addTest(unittest.makeSuite(HPMySQLSrcZODBConvertTests))
+    suite.addTest(unittest.makeSuite(TestOIDAllocator))
 
     from relstorage.tests.blob.testblob import storage_reusable_suite
     from relstorage.tests.util import shared_blob_dir_choices