[ZODB-Dev] Tracking down a freeze (deadlock?)

[Dieter Maurer]

Tim Peters wrote at 2005-2-25 10:01 -0500:
> ...
>[Florent Guillaume]
>>>   File "/opt/zope/lib/python/ZODB/Connection.py", line 257, in _setDB
>>>     self._flush_invalidations()
>>>   File "/opt/zope/lib/python/ZODB/Connection.py", line 552, in
>>> _flush_invalidations
>>>     self._cache.invalidate(self._invalidated)
>>>   File
>>>
>"/appli/zeo/zeocli-192.168.106.6-8080/Products/DICOD/DICODMailingList.py",
>line 125, in __del__
>>>   File "/opt/zope/lib/python/ZODB/Connection.py", line 599, in setstate
>>>     invalid = self._is_invalidated(obj)
>>>   File "/opt/zope/lib/python/ZODB/Connection.py", line 617, in
>_is_invalidated
>>>     self._inv_lock.acquire()
>>>
>>> Hm I think I can answer that one. A persistent object is not supposed to
>>> have a __del__ that accesses the ZODB right ? Otherwise, well, we see
>>> what happens.
>
>[Dieter Maurer]
>> On the other hand, it should not cause a deadlock.
>
>Why is that?  As far as I'm concerned, ZODB doesn't support persistent
>objects with __del__ methods -- it was never intended to.

If it does not support objects with "__del__" method,
it should

 1.  clearly state so

 2.  raise an exception when it meats one

It is far too expensive to analyse a deadlock's cause
such that silently deadlocking is not "friendly"...


On the other hand, apparently not all "__del__" methods
seem to cause a deadlock (otherwise, the tests for
the infinite look would have revealed it).
Thus, maybe, Florent's analysis is not yet complete...


>2. Avoiding the deadlock is only the tip of the iceberg.  Locks are
>   meant to ensure invariants, and the latter are rarely documented
>   in ZODB.  To ensure that invariants aren't violated when switching
>   to a reentrant lock requires trying to figure out what all the
>   intended invariants actually are, then checking every possibly
>   relevant code path to ensure that those invariants remain satisfied
>   in reentrant cases.

In this special case, the lock's purpose is documented:
ensuring atomic processing of invalidations.

As such invalidations always come from a different thread,
a reentrant lock would not change anything for them.

>So, ya, it's a matter of seconds to change from a Lock to an RLock here, and
>a matter of perhaps 20 minutes to write and debug a good new test case(s),
>but every ZODB user would pay runtime expense for that forever after,

Yes, but we need to keep this in perspective:

     This lock is usually acquired in connection with an
     expensive operation (the loading of an object, the commit
     of a transaction (maybe remote) and the opening a
     connection). Compared to the "surrounding" operations,
     the locking cost seem to be minor.

>and it
>may or may not introduce subtle new bugs (depending on the analysis in #2,
>and also on whether that analysis is wholly correct).  If the benefit is to
>stop a deadlock in application code Florent is inclined to believe is
>incorrect anyway, it sounds like a losing set of tradeoffs to me.

If we really have the feeling that persistent objects
with "__del__" can cause deadlock, we should probably stamp
them out in a future ZODB version.
This would give a clear exception rather than a difficult to
analyse deadlock -- okay: at the cost of an additional test.

-- 
Dieter