Price guardian

[christopherlam]

Rewrite for #1403

Adds mechanism for guarding SCM objects referring to price. gnc:price-ref will guard the price. As long as the SCM object is alive, the price ref will be increased; when gc runs, the guardian will appropriately unref prices.

If a report which accesses price is run, and gc hasn't triggered yet, then the price ref would not be decremented yet. Hence the call to scm_gc() after running a report.

[jralls]

This bugs me. Does (gnc:price-ref price) put price or *price (in C's meaning, Guile having no way to express it) on the guardian's list? If it's *price then no problem; otherwise this will leak the original *price and if trans-price will get an extra unref.

[christopherlam]

well, the reports shouldn't generally call gnc-price-ref/unref, and shuould leave it to the guardians. I don't know whether it calls price or *price.

[jralls]

They don't generally, only the two portfolio reports do. Nothing in Scheme except test-engine-extras.scm writes to price objects, so one option would be to copy the few things that reports need from prices (currency, commodity, time, and value) and not swig any of the price API.

Do you understand the problem with (set! price trans-price) if the guardian is using the addresses of price and trans-price instead of the addresses that they point to (e.g. *price)?

[christopherlam]

Do you understand the problem with (set! price trans-price) if the guardian is using the addresses of price and trans-price instead of the addresses that they point to (e.g. *price)?

Not quite.

[jralls]

There are two GncPrice pointers, p1 and p2. price is initialized to p1 and trans-price to p2. price-guardian is called on price and trans-price when each is initialized, then (set! price trans-price) happens and both price and trans-price equal p2.

If (price-guardian price) causes the guardian to keep track of p1 (what I called the '*price' scenario) then after the set! call it's garbage-collected and returned and unreffed by reclaim-prices. Later when both price and trans-price go out of scope (maybe at the end of the report render)p2 is garbage collected and can be unreffed by reclaim-prices. All good.

But if (price-guardian price) keeps a reference to price (the 'price' scenario) then it's not guarding p1. It doesn't return anything until price or trans-price go out of scope, and when they do it returns p2 for each one, so p1 leaks and p2 gets unreffed twice. Not good.

[christopherlam]

I see slightly and don't know the answer.

[jralls]

Then you need to find out. If it's the latter case then the set! has to go, it's a potential use-after-free crasher.

Alternative: Remove all of the price refs and unrefs, no guardians. I don't think any of these calculations will be interrupted by a status bar update, and even if they can the user would have to somehow queue a price-delete to run during an idle for a price to become invalid while the report is loading. Pretty darn unlikely.

[christopherlam]

Alternative: Remove all of the price refs and unrefs, no guardians. I don't think any of these calculations will be interrupted by a status bar update, and even if they can the user would have to somehow queue a price-delete to run during an idle for a price to become invalid while the report is loading. Pretty darn unlikely.

So, #1403 resurrected.

[jralls]

OK, probably safer than this. But copying it into a scheme record would be safer still.

[christopherlam]

I had another idea to move this guardian to engine.i and disallow gnc_price_ref/unref from being swigged.

[richardcohen]

@christopherlam did you try using swig %refobject ?
https://www.swig.org/Doc3.0/SWIGPlus.html#SWIGPlus_ref_unref

[christopherlam]

@christopherlam did you try using swig %refobject ?
https://www.swig.org/Doc3.0/SWIGPlus.html#SWIGPlus_ref_unref

Good find... unfortunately I have no idea how this works.

[jralls]

swig %refobject

It's for "legacy C++", meaning before C++11. Modern C++ uses std::shared_ptr instead of hand-rolling ref counting. It also appears to require ref and unref to be member functions, so it will work only with a C++ class, and GncPrice is a GObject class. That aside a check of the source code suggests that it isn't even fully implemented.

Having reread again the Guile Memory Management docs I'm pretty sure that guile's GC only knows about guile objects (the 'price' model). So guardians are a fine way to handle lifetime but the guarded variable can't be mutated: No set!.

[jralls]

I had another idea to move this guardian to engine.i and disallow gnc_price_ref/unref from being swigged.

It's nicer because it hides the guardians from the report code, but it still has the reference problem.

Alternative: Remove all of the price refs and unrefs, no guardians. I don't think any of these calculations will be interrupted by a status bar update, and even if they can the user would have to somehow queue a price-delete to run during an idle for a price to become invalid while the report is loading. Pretty darn unlikely.

So, #1403 resurrected.

No, #1403 wanted to remove the memory management from gnc-pricedb.c, a terrible idea. I'm just suggesting to remove it from the two portfolio reports. Copying the price info into Scheme would reduce the risk still further; doing so in wrapped C code that refs the gnc_price, makes the copy, then unrefs the gnc_price would be safest.