Custom _p_invalidate not called when deleting _p_changed

[Jacob Holm]

From persistent/interfaces.py:

    There are two ways to invalidate an object: call the
    _p_invalidate() method (preferred) or delete its _p_changed
    attribute.

It turns out that these two are not equivalent when the object has a custom _p_invalidate method. Specifically, the custom _p_invalidate method is never called when you do delete the _p_changed attribute. (Similarly, a custom _p_activate is never called when unghostifying an object, but I don't have a use case for that that isn't already covered by having a custom __setstate__)

Looking at cPersistence.c it is immediately obvious why. The way things are implemented, Persistent._p_invalidate() uses the equivalent of "del self._p_changed" which of course doesn't call _p_invalidate again. Instead it sets the state to UPTODATE and calls _p_deactivate. (Why it does that is another matter, I think lying about the state when potentially calling user code is *BAD*)

I think it would be better if that was reversed so Per_set_changed would call the objects (possibly custom) _p_invalidate, which would then set the state and call _p_deactivate itself. This would not only fix the immediate problem, but also make the control flow more obvious. (Modifying _p_changed dispatches to either _p_deactivate or _p_invalidate. _p_invalidate sets state to UPTODATE, calls _p_deactivate, and then forces the object to become a ghost)

Would a patch along those lines be acceptable for 3.10? Or should I rather concentrate on a patch that eliminates all *uses* of "del p._changed" from ZODB (a one-line change in ZODB/Connection.py), and hope that none of my other dependencies use it?

[Jacob Holm (jacobholm)]

I found a workaround that seems to cover most cases. Just override the _p_changed property in your class like this:

class P(persistent.Persistent):

    _p_changed = property(
        persistent.Persistent._p_changed.__get__,
        persistent.Persistent._p_changed.__set__,
        lambda self: self._p_invalidate(),
        "Workaround for https://bugs.launchpad.net/zodb/+bug/586526",
        )

This should work unless there is code out there that uses persistent.Persistent._p_changed.__delete__ or the c function cPersistence.c:Per_set_changed directly. I'm guessing there is not a lot of code that uses either.

I'm still interested in working on a patch to fix this, but only if I know that it has a chance.

[Jim Fulton (jim-zope)]

On Thu, May 27, 2010 at 4:00 PM, Jacob Holm <email address hidden> wrote:
> Public bug reported:
>
> >From persistent/interfaces.py:
>
>    There are two ways to invalidate an object:  call the
>    _p_invalidate() method (preferred) or delete its _p_changed
>    attribute.

And, of course, ZODB itself does the later. :)

> It turns out that these two are not equivalent when the object has a
> custom _p_invalidate method.

Gaaaa. Why do you want to override _p_invalidate?

>   Specifically, the custom _p_invalidate
> method is never called when you do delete the _p_changed attribute.
> (Similarly, a custom _p_activate is never called when unghostifying an
> object, but I don't have a use case for that that isn't already covered
> by having a custom __setstate__)
>
> Looking at cPersistence.c it is immediately obvious why.  The way things
> are implemented,  Persistent._p_invalidate() uses the equivalent of "del
> self._p_changed" which of course doesn't call _p_invalidate again.
> Instead it sets the state to UPTODATE and calls _p_deactivate.   (Why it
> does that is another matter, I think lying about the state when
> potentially calling user code is *BAD*)
>
> I think it would be better if that was reversed so Per_set_changed would
> call the objects (possibly custom) _p_invalidate, which would then set
> the state and call _p_deactivate itself.    This would not only fix the
> immediate problem, but also make the control flow more obvious.
> (Modifying _p_changed dispatches to either _p_deactivate or
> _p_invalidate.  _p_invalidate sets state to UPTODATE, calls
> _p_deactivate, and then forces the object to become a ghost)
>
> Would a patch along those lines be acceptable for 3.10?

This whole implementation is such a mess wrt custom _p_invalidate and
_p_deactivate that I really don't want to touch it until there's a
significant do over.

The way this should have been done was to have something like:
_p_release_state that is called by _p_invalidate and _p_deactivate.

Better yet, I might even replace _p_invalidate with a flag indicating
whether the object was ghostifiable.

In any case, I have very little interest in fooling with the C
persistence implementation at this time.

>   Or should I
> rather concentrate on a patch that eliminates all *uses* of "del
> p._changed" from ZODB  (a one-line change in ZODB/Connection.py), and
> hope that none of my other dependencies use it?

That might be better, although I don't think this is the only place
where del is used.

Jim

--
Jim Fulton

[Jim Fulton (jim-zope)]

Hm, I left the impression that I'm interested in a patch for this. :)

This is in an area of ZODB I really don't want to mess with until I redo persistence.

Why do you want to override _p_invalidate?

[Jacob Holm (jacobholm)]

> Why do you want to override _p_invalidate?

I wanted a persistent object that, dependent on its own state may be unghostifiable. A _p_immortal flag that is saved as part of the object state would be good enough.

Based on the documentation, I thought something like this would work.

  class P(persistent.Persistent):

    def __init__(self, immortal=False):
      self._immortal = immortal

    def _p_invalidate(self):
      immortal = self._immortal
      super(P, self)._p_invalidate()
      if immortal:
        self._p_activate()

But it doesn't, due to the problem I reported.

The reason I want this is to be able to maintain a memory-only structure along with my persistent object. It may be expensive to recreate from scratch, but can be updated cheaply if done often enough (like once every time we are told of changes to the persistent object). The immortal flag would be used only when the structure *would* be expensive to recreate. (As determined either by some formula or by a human operator).