_p_changed is stuck if register() fails

[PJE]

If an error occurs during the call to a data manager's register() method when setting _p_changed, the persistent object's po_state is left in the "CHANGED" state, even though the object has not actually been changed. The state should remain "UPTODATE", otherwise a subsequent change to the object will not result in a repeat call to the data manager's register() method.

When an object's state is changed directly, cPersistence does not update po_state until register() has successfully returned. The code for handling direct assignment to _p_changed should do likewise. Moving the line:

     self->po_state = CHANGED;

*after* the call to _PyPersist_RegisterDataManager should do the trick.

Note, however, that if a data manager's register() should happen to modify the object, this will result in potentially unbounded recursion. This should be documented in the IPersistentDataManager interface, since it can happen for attribute assignments now.

I'd be more than happy to handle the necessary fixes and docs in CVS.

[PJE (pje-telecommunity)]

Also note that _PyPersist_RegisterDataManager() itself sets
po_state to CHANGED even if register() raised an exception. It's somewhat questionable whether it has any business setting po_state at all; none of the places that call it inside cPersistence rely on it doing this; they all set po_state to CHANGED themselves.

It also appears as though there is considerable redundant code for state management surrounding the places that call _PyPersist_Load(); but the logic used in each case is slightly different. At this point, I'm not 100% positive I've caught all the places that suffer from this same logic error (i.e., leaving po_state CHANGED when an error occurs). :(

[Jeremy Hylton (jeremy-alum)]

I don't think either possibility is a good one. If register() fails, then the object shouldn't be left in the UPTODATE state. That would mean register() would get called again, but it would likely raise an error again. You're right that it should be marked as CHANGED either, since that means a transaction could commit without catching the object's modifications.

Jim and I chatted a bit and concluded that we should have an INCONSISTENT state, which is neither UPTODATE nor CHANGED. Any operation on an INCONSISTENT object will cause an error. The goal is to cause the current transaction to fail as quickly as possible. However "del obj._p_changed" gets spelled should be the only way to reset an object in this state.

[PJE (pje-telecommunity)]

> = Comment - Entry #3 by jeremy on Dec 20, 2002 6:04 pm
>
> I don't think either possibility is a good one. If register() fails,
> then the object shouldn't be left in the UPTODATE state. That would mean
> register() would get called again, but it would likely raise an error
> again.

I don't really see why it raising an error again would be a problem. If you try to modify the object again, and the problem that made register() barf hasn't been fixed, you *should* get an error again.

> You're right that it should be marked as CHANGED either, since
> that means a transaction could commit without catching the object's
> modifications.

Yes, that's exactly the problem I was bitten by. I'd get an error in register(), fix the problem, modify the object again, everything seemed okay, and then it didn't get saved.

> Jim and I chatted a bit and concluded that we should have an INCONSISTENT
> state, which is neither UPTODATE nor CHANGED. Any operation on an
> INCONSISTENT object will cause an error. The goal is to cause the
> current transaction to fail as quickly as possible. However "del
> obj._p_changed" gets spelled should be the only way to reset an object in
> this state.

While I agree with this in principle, the fix I've proposed is a few minor changes that result in an improvement on the current state, ensuring that the error does not pass silently if code attempts a retry. It isn't merely a theoretical issue, since I've actually been burned by it. So I think it would be better to have a "now is better than never, practicality beats purity" partial solution available. :)

In the last few months I've been using a forked copy of cPersistence.c with the fixes for this and also all of the Collector #86 fixes as well, so I'm pretty comfortable with the practicality of the solution. I'd prefer to avoid continuing the fork any further, as it would hamper interoperability between PEAK and Zope if the differences keep increasing. So if there are no objections to making the intermediate fix, I'll go ahead and add it sometime next week. Thanks.

[PJE (pje-telecommunity)]

Fix and unit tests checked into cvs.zope.org yesterday; this item can be closed now by someone with access to the Collector.

[Jeremy Hylton (jeremy-alum)]

Changes: submitter email, edited transcript, new comment

I'd rather leave this bug report open until a more complete solution is implemented.

[Jim Fulton (jim-zope)]

Status: Pending => Deferred

We are no longer working on zodb 4