[ZODB-Dev] RE: [Zope-Annce] ZODB 3.2.4 release candidate 1released

[Chris McDonough]

(Finally clearing this issue up in my head, so I won't respond to the
other similar messages in this thread)

On Fri, 2004-09-10 at 02:59, Tim Peters wrote:
> [Dieter Maurer]
> >>> Let's cast it into a slightly different picture:
> >>>
> >>>     try:
> >>>       try:
> >>>         # perform modifications on persistent state
> >>>       except ...
> >>>         # do something that does not abort the transaction
> >>>         # and does not raise an exception
> >>>       ...
> >>>       commit()
> >>>     except:
> >>>       abort()
> >>>       ...
> >>>
> >>> Should the "perform modifications" result in any exception, then the
> >>> exception is likely to leave the state inconsitent. Catching the
> >>> exception causes "commit" to be called which makes the inconsistent
> >>> state persistent.
> 
> [Chris McDonough]
> > I don't think there's anything ZODB can do about the above case, is
> > there?  I'm so confused my head is spinning, to be honest. ;-)
> 
> There isn't, and don't forget that application invariants can be broken even
> if no exceptions occur (let alone get suppressed), and even if conflict
> resolution doesn't get involved.  It depends on the app and its invariants.
> It makes it hard to know exactly which problem(s) we're trying to solve
> here.

Yup.  That's a good point.  I think the current policy is:

a) Code intends to effect persistent state in some known way.
   If the way the code is meant to effect the state works
   in a well-understood way, app data consistency can typically
   be maintained, otherwise it may not be not be.  That's life.

b) The developer is responsible for ensuring app data consistency
   related to "a" in most if not all circumstances.  This is
   a big responsibility, although most code seems to manage it
   OK.  This is explicitly not ZODB's responsibility.

c) There are exceptions to "b" for cases where it's improbable
   or impossible to predict the codepath due to ZODB implementation
   artifacts (eg. ReadConflictError). In these cases it is reasonable
   to expect ZODB to "help" by rendering a transaction uncommittable,
   preventing the developer from needing to put a try: except: 
   DeathByChris block around every statement that does attribute access
   on persistent objects.

   Relatedly, as a nicety, wherever a "fatal" exception is
   raised (one that is not supposed to be caught by anything except 
   perhaps something that catches it explicitly, and one that is only
   meant to be raise by the ZODB framework) and when it's possible to
   do in a sane way, the associated transaction should probably be
   rendered uncommittable by the ZODB framework.  (POSKeyError,
   VersionLockError, etc)

d)  A corollary: it would be nice if some exceptions couldn't be
    caught except explicitly.  It's nice in Zope's case because it
    removes a bit of data integrity maintenance responsibility from
    both the developer who uses Zope and from the developers who create
    ZODB and into a centralized top-level exception handler within
    the publisher, which can handle these sorts of things in a
    well-defined way.

e)  A final corollary: bare excepts should be prevented in app code
    as well as things like hasattr which has same effect.

> > But I *think* that it's only in the case that the exception which is
> > raised is a ConflictError or a commit failure does ZODB have any
> > responsibility whatsoever, and that's only because both are very fatal
> > and there's no way to prevent either from being caught inappropriately by
> > overeager exception handlers.  The machinery to make sure that nothing
> > "bad" happens to persistent state at commit time due to inappropriately
> > catching a ZODB-related exception is basically a hack, isn't it?
> 
> I keep saying ReadConflictError because that's the only exception ZODB
> arranged to "make stick" before 3.2.4c1 (ConnectionStateError got added
> then, and persists until abort()/begin()/commit()).  There's no special
> treatment of, e.g., VersionLockError or POSKeyError.  Maybe there should be;
> I don't know.

Probably, they are no different than any other errors.  Basically, in
the current state of affairs, the raiser of any "fatal" exception
whatsoever could be presumed responsible for rendering the transaction
uncommittable before doing the raise.

I suppose it would be a good thing to define what "fatal" means.  To me,
from a Zope-centric perspective, it means exceptions that are only
supposed to be caught by the Publisher but might be caught by
intermediate code.

> AFAIK, write conflicts can happen only during commit(), and *any* unhandled
> exception during commit() aborts the current transaction, so write conflicts
> aren't special either.

Violent agreement.

> WRT ReadConflictError, yes, that's made sticky via a hack spread out over 3
> places in Connection.py.  It's so hackish that 2 of those 3 places had bugs
> (which we discovered and fixed last week -- but they were minor bugs, one
> with no visible consequences, and the other I bet never occurred before I
> added a strained test for it).
> 
> > If Python had a separate exception hierarchy for errors that a bare
> > except: (or C equivalent, if it exists, and hasattr too!) couldn't catch,
> > I think ZODB could even relinquish responsibility for doing anything
> > special when ReadConflictError happens.
> 
> Indeed, the only reason ZODB does anything special with ReadConflictError is
> because you Zope weenies kept whining about it until Jeremy gave up asking
> you to learn how to write decent code <wink>.

Yeah, well we clearly still haven't done that yet, but doing something
special with RCE still seems like a fine idea to me! ;-)

> > By that time we'll all have MVCC anyway, I guess. ;-)  But it would seem
> > to also prevent ZODB from needing to do anything special when a commit
> > fails in the way we're jawing on about in the other thread too.  It would
> > also probably be more efficient, because the transaction wouldn't need to
> > hopelessly go on doing work that is preordained to fail at commit time.
> 
> That's the discussion on python-dev, which gained a little momentum, but
> seems to have fizzled out already.  At the C level this is very difficult,
> because PyErr_Clear() calls are *all over the place* (not just in Python!
> ZODB 3.2.4c1's C code calls it 66 times).  Many of those are more-than-less
> obviously safe, but you can burn an hour trying to guess about just one of
> the others.  Like:
> 
>     oid = PyObject_GetAttr(object, py__p_oid);
>     if (!oid) {
> 	PyErr_Clear();
> 	goto return_none;
>     }
> 
> __getattr__ can do anything, so this is basically a duplication inside
> ZODB's C code of the same problems Python's __builtin__.hasattr() creates.

Ya.

> Armin Rigo has a speculative patch pending that tries to get "dangerous"
> exceptions raised *even if* PyErr_Clear() gets done at the C level.

This seems like a sane thing as long as you can define what "dangerous"
is.

>   Haven't
> had any time to look at that, though.  There's no cause to hope for quick
> relief here, anyway.

Right.

Thanks!

- C