Re: Discrepancy between Java and C is settled methods

2013-03-08 Thread Rafael Schloming
On Fri, Mar 8, 2013 at 4:54 AM, Phil Harvey p...@philharveyonline.comwrote:

 Thanks for the reply.  I haven't yet figured out whether I completely agree
 with your reasoning, but it is really useful to see the intention behind
 these functions written down in black and white.

 Keith and I are going to be writing more tests for Delivery quite soon, so
 I'd like to defer the full resolution of these questions (including
 JNIDelivery state/memory management issues) until then.  For now, I will
 just document this discrepancy in a TODO in the Delivery.isSettled JavaDoc.

 Also, would you be able to add a comment to pn_delivery_settled to hint
 that it is not simply the property accessor counterpart to
 pn_delivery_settle?


I can do that.

Looking into this has made me start to think pn_delivery_free() could be a
better name for what pn_delivery_settle() is actually doing. There is an
open JIRA arising from someone not realizing that pn_delivery_settle
renders the delivery pointer invalid, so one possibility would be to change
pn_delivery_settle to pn_delivery_free. This would help signal the
appropriate memory management aspects of the operation, and would break the
settle/settled pairing.

--Rafael


Re: Discrepancy between Java and C is settled methods

2013-03-07 Thread Rafael Schloming
On Wed, Mar 6, 2013 at 10:27 PM, Phil Harvey p...@philharveyonline.comwrote:

 Ok, I hadn't realised that settling a delivery in proton-c had such a
 significant effect on its state.

 Does this imply that it's illegal for an application to use a delivery in
 any way once it's called pn_delivery_settle? If so, we should document this
 in in the header file.


It does.

Also, any views on my function name suggestions?


I'm not super keen on what you're proposing for the C API. I'll try to
describe my reservations, but I'm doing a bit of thinking out loud here, so
bear with me.

Right now pn_delivery_readable, pn_delivery_writable, pn_delivery_updated,
and pn_delivery_settled are all flags indicating a situation that has
arisen asynchronously due to remote activity, and hence very related to the
notion of the work queue. If anyone of those flags becomes true then the
delivery will appear in the work queue. These flags, however, are also
influenced by local actions, and in that respect they aren't the same as
the remote/local state fields that appear elsewhere in the API.

For example local/remote source/target, local/remote container,
local/remote hostname, etc, directly correspond to state values held by
both the local and remote endpoint. In each of these cases they are only
ever set based on what the remote peer reports, and short of getting the
remote peer to report something else, there is no way to effect them
through the API. The above flags, however, are also influenced by local
actions, e.g. a delivery might become readable when the remote peer
supplies message data, but it stops being readable when you read whatever
is available, likewise the delivery's remote state might be updated
asynchronously, however the local app can access the updated value and then
call pn_delivery_clear() to clear the updated flag.

In the case of settlement there is another difference from the
local/remote_foo pattern as used elsewhere. In all cases of the
local/remote_foo pattern remote_foo is a fact currently being remembered by
the remote endpoint, whereas in the case of settlement, the remote endpoint
has actually destroyed all knowledge of the delivery, and so the flag is
actually indicating the absence of any facts at all at the remote endpoint
rather than, e.g., indicating that the remote endpoint is holding onto a
delivery object with a settled flag whose value is now true. In other words
settled is actually a verb rather than a noun as remote_settled might
suggest.

I suspect something of this intuition was present on the Java side when the
current names were chosen from the choice of remote*ly*Settled() rather
than remoteSettled().

One possibility for naming would be to adopt the remotely_settled
terminology on the C side, however I'm not mad on this either as it breaks
what is currently a common pattern between pn_delivery_update/updated, and
pn_delivery_settle/settled, the former being the local verb, and the latter
being indication that the remote verb has occurred. Another possibility
would be to simply eliminate the isLocallySettled() notion from the Java
API and make isSettled() a synonym for isRemotelySettled().

Probably a very relevant question though before trying to sort out naming
consistency is whether the semantics are actually compatible. It occurs to
me that if you're backing the Java Delivery interface with a JNIDelivery
you may well get some very strange behaviour if you keep around references
to locally settled deliveries. I'm guessing the settled deliveries will
probably get reused by the C impl and their tags/state/etc will randomly
start changing based on their new usage. I think a correctly wrapped JNI
layer would need to discard any pointer to the C delivery and therefore
need to throw some kind of exception when any of the methods on Delivery
were actually called post settlement.

I'm also wondering what Delivery.free() actually does and given that there
isn't a Delivery.isFreed() if perhaps Delivery.free() is more analogous to
what pn_delivery_settle() is actually doing.

Apologies for the long and rambling reply. It's been a long and rambling
kind of day. ;-)

--Rafael


Re: Discrepancy between Java and C is settled methods

2013-03-05 Thread Rafael Schloming
On Tue, Mar 5, 2013 at 11:36 AM, Phil Harvey p...@philharveyonline.comwrote:

 There's a confusing difference in the meanings of the delivery is settled
 methods in proton-j (Delivery.isSettled) and proton-c
 (pn_delivery_settled): their return values represent the local and remote
 values respectively. proton-j has a separate remotelySettled() method,
 whereas proton-c appears to have no way of accessing the local state.

 I'd like to modify one or both apis to resolve this semantic difference.
 There are clearly a number of options.

 My favourite is to modify the proton-c function to return the local value,
 and add a new function to return the remote one. This is consistent with
 the other functions that have local and remote counterparts, eg
 pn_link_source and pn_link_remote_source. If this change in proton-c api
 semantics is too abrupt, maybe we could just deprecate the existing
 function and method and add new ones that are explicit about their
 local/remote meaning.

 What do people think?


I don't think it's possible to modify the C API in that way. A locally
settled delivery is actually considered freed (really it's returned to a
pool), so it's not possible to actually query that state, i.e. if your
modified pn_delivery_settled(pointer_to_delivery) where to ever return true
then that means you just passed it a pointer to garbage memory.

--Rafael