On 07/16/2015 02:40 PM, aconway wrote:
The fix mentioned above has this, which make no sense under traditional refcounting:pn_incref(endpoint); pn_decref(endpoint);
Note that this is not added as part of my fix, it is already there. The simple explanation is that it is a trick to get a finalize function for the object in question to be re-run (when it was previously cut short). The broader context is that the reference counting is not used everywhere internally (it was a more recent addition), causing some additional twists and turns in the logic.
Traditionally, this sequence is a no-op if the caller owns the reference and is illegal if not. We need a clear statement of the semantics of such sequences: when it it legal and what does it mean? From the fix it seems to be related to the "freed" and "referenced" members, what exactly is the relationship?
The 'freed' flag indicates that the pn_xxx_free() call was made. This is not always the case. There is a mode of use, e.g. in the python binding, that use inc-/dec- ref instead. I'm not as clear on the design behind the 'referenced' flag, but it is set to false when the finalizer is cut short.
The original fix for PROTON-905 triggered the re-run of the finalizer whenever the reference count was 0 and a session or link was moved out of the set of modified items. From the description it was only intended to do this where the object in question had been freed and had had its finalizer cut short. Hence the addition in my proposed fix of that extra qualification.
As it is, the code currently on master, pending the 0.10 release is *incompatible* with qpid-cpp. So either the original fix needs to be reverted or it needs to be corrected in some way (e.g. along the lines I have suggested).
The reference counting logic may not match the ideal, but we can't postpone a fix for the current issue pending some nicer overall solution. We can avoid it, by backing out the problematic previous commit, or we can adjust that commit.
