[ZODB-Dev] Race condition in Connection.close

[Tim Peters]

[Marius Gedminas]
>> I think I found a race condition in Connection.close in the current
>> ZODB 3 CVS:

I think you have too -- good eye!  It was apparently introduced in a
"cleanup patch" several weeks ago.

>>     # ZODB/Connection.py
>>     ...
>>     class Connection(...):
>>         ...
>>         def close(self):
>>             ...
>>
>>             # Return the connection to the pool.
>>             if self._db is not None:
>>                 self._db._closeConnection(self)             # 1
>>                 self._db = None                             # 2

Unfortunately, I have so far been unable to write a test that triggers the
problem on my box, although it's very easy to trigger if I stick a short
sleep() between #1 and #2.

...

>> I'll try testing the obvious fix tomorrow and see if this fixes the
>> errors I saw.
>>
>>             # Return the connection to the pool.
>>             if self._db is not None:
>>                 db = self._db
>>                 self._db = None
>>                 db._closeConnection(self)

That looks good to me, except it's in dire need of a comment.  I'll add one.

> Running the tests with this change applied did not show any errors
> after 670 runs.
>
> Should I commit the fix to ZODB CVS?

I'll do it.

> I've got no idea how to write a unit test for it.

Me neither, and unit tests really don't work well for thread-race bugs even
when they're easy to provoke.

> Well, maybe stub _closeConnection and check that the connection that
> is passed has its _db attribute set to None.

Possibly.  I'll think about it some more.  Offhand, it's not clear that "_db
is None" is a sensible precondition for _closeConnection(), and it may make
more sense for _closeConnection to None-out _db itself (in which case
Connection.close() could assert that self._db is None after
_closeConnection() is called).  The vague intuition there is that the race
"is really inside" DB methods, not Connection methods.