[Gnucash-changes] r13130 - gnucash/trunk/src/engine - Replace check_open() in setter API with internal qof_{begin, commit}_edit().

[Derek Atkins]

Quoting Chris Shoemaker <c.shoemaker at cox.net>:

>> > Modified: gnucash/trunk/src/engine/Transaction.c
>> > ===================================================================
>> > --- gnucash/trunk/src/engine/Transaction.c	2006-02-06 06:45:25 UTC 
>> (rev 13129)
>> > +++ gnucash/trunk/src/engine/Transaction.c	2006-02-06 16:18:52 UTC 
>> (rev 13130)
>> > @@ -1626,7 +1626,7 @@
>> >    gint fraction, old_fraction;
>> >
>> >    if (!trans || !curr || trans->common_currency == curr) return;
>> > -  check_open (trans);
>> > +  qof_begin_edit(QOF_INSTANCE(trans));
>>
>> I think you want to be using xaccTrans{Begin,Commit}Edit here, and not
>> qof_{begin,commit}_edit.  The resaon is clear in a moment...
>
> Just so we're on the same page: I see the two as operating at very
> different levels of abstraction.  The qof_{begin,commit}_edit is only
> for storage coherency - it knows nothing about data consistency.  And
> xaccTrans{Begin,Commit}Edit is the engine's external API for allowing
> users to create consistent Transactions.  It _does_ care about data
> consistency.

This goes back to the conversation we had on IRC the other night.  Right
now the API isn't very clear about the distinction between storage coherency
and data consistency.   It's muddied even worse due to the way the code
logs changes.

As we agreed on IRC, each setter operation needs to use begin/commit
semantics to make sure you have data consistency.   Ideally we'd also
log when the change is committed.   For the short term I'm willing to
potentially miss some log messages.

HOWEVER, we do need to deal with the fact that qof_commit_edit() doesn't
destroy an object.  So that means we really do need to use xaccTransCommitEdit
at least in xaccTransDestroy().

>> [snip]
>> > -  trans->inst.do_free = TRUE;
>> > +  if (!xaccTransGetReadOnly (trans) ||
>> > +      qof_book_shutting_down(trans->inst.book)) {
>> > +      qof_begin_edit(QOF_INSTANCE(trans));
>> > +      trans->inst.do_free = TRUE;
>> > +      qof_commit_edit(QOF_INSTANCE(trans));
>> > +  }
>>
>> qof_commit_edit() knows nothing about inst.do_free.  The do_free is
>> only handled by the object's Begin/Commit edit API, not by the lower
>> level QOF begin/commit edit API.
>
> The macro versions actually do handle inst.do_free.  There's terrible
> confusion about responsibility delegation between engine objects and
> QOF, because the transactional semantics offered by QOF are
> inconsistent.

The problem is that qof_begin_edit() and qof_commit_edit() were added
to the external API.   In general you're just NOT supposed to use them,
and you're supposed to use the object-specific begin/commit edit.
The macro is there to help you implement your object-specific begin/commit
edit functions, because frankly they're all pretty much the same.

> Probably what _should_ happen is that the engine-level object should
> maintain its own "deleteme" flag, and then use that during the
> engine-level CommitEdit to set the QOF-level do_free, knowing that
> once qof_commit_edit() is called on a do_free marked object, it's no
> longer valid.

Nah, no need to do that.

>> So this can lead to a memory leak.
>
> Well, I don't think it will leak if the xaccTransDestroy() is
> surrounded by engine-level begin/commit.

IFF!  If it's not, well, the object wont get destroyed.  This is what
the check_open() was for in that particular function.  ;)

> The problem with using engine-level begin/commit inside the setter
> api is that it may (and does, for Transaction) enforce constraints
> that aren't valid after every field-set.  In fact, the
> xaccTransCommitEdit will happily free any Trans that doesn't have any
> split children yet.  That may make sense for engine-level data
> consistency constraints, but it sure doesn't make sense to enforce
> that inside xaccTransSetDescription().

That's arguably true...  But remember that those checks are only performed
when the commit nest level reaches zero.  The engine user should be
calling BeginEdit() before filling in the transaction so it shouldn't
be an issue.    But yes, the db-transactional semantics aren't very clear
and unfortunately you ran smack-dab into the middle of the incoherencies
of the API.  :(

>> Also, by calling the qof command directly you've now made it such
>> that you could lose transaction log information.  The log is written
>> out in xaccTrans{Begin,Commit}Edit so by bypassing that you lose
>> logging info.
>
> I don't think you want to log _in between_ the
> xaccTrans{Begin,Commit}Edit, only on the edges.  This is especially
> clear when using "temporary" engine objects.  The gui very much wants
> to distinguish between "Ok, I'm going to edit a real-live important
> object. Please ensure everything remains consistent and coherent."
> and "Just give me a temporary (new) object to play with.  Don't ever
> save it anywhere or log it anywhere or enforce any inter-object
> consistency on it.  Pretty much don't touch it.  I'll free it when I'm
> done.  Thank-you-very-much."

The engine really has no concept of a non-saved object or temporary object.
Perhaps this is a bug.  I don't know.  But it's certainly true that
you cannot create a temporary object.  IMHO this is a GOOD THING -- it
prevents accidental data loss.

> I think the best way to make that distinction is to wrap the former
> case in the engine-level begin/commit, but not the latter.
>
> I know the current code is far from ideal, but I'm really struggling
> to see a way to improve it incrementally without some invasive changes
> to QOF.  If you see a path forward, (and it's not one that either a)
> requires all external API users to wrap a parameter setting in between
> {Begin/Commit}Edit or b) enforces inter-object constraints every each
> and every parameter setting) I'm willing to implement it.

I see no need to make /ANY/ changes to QOF.  QOF is fine.  It's the
users of QOF (Transactions, in this case) that are causing the problem.
A simple change would be a couple new private functions that wrap the
QOF object storage consistency and the logging, and then you can use those
internally..   But it WILL share a significant amount of code with
xaccTransBeginEdit and xaccTransCommitEdit -- so it may just make sense
to refactor those functions, or use the QOF BEGIN/COMMIT macros.

> -chris

-derek

-- 
       Derek Atkins, SB '93 MIT EE, SM '95 MIT Media Laboratory
       Member, MIT Student Information Processing Board  (SIPB)
       URL: http://web.mit.edu/warlord/    PP-ASEL-IA     N1NWH
       warlord at MIT.EDU                        PGP key available