Re: [Anima] comments on draft-ietf-anima-grasp-api

[Brian E Carpenter <[e-mail]>]

With notes added while starting to update the draft:

On 07-Aug-19 14:28, Brian E Carpenter wrote:
> Thanks very much Michael. Some partial responses below.
> 
> On 07-Aug-19 05:24, Michael Richardson wrote:
>>
>> I read draft-ietf-anima-grasp-api from the expired drafts list.
> 
> Right, the -03 draft expired while we were in Montreal. Our plan
> is to make the next update after the two promised reviews arrive.
> 
>> I think that the event-loop architecture is different than a
>> polling architecture.  I agree that given an event-loop architecture,
>> that one can build a polled architecture, but the converse is not true.
>> The event-loop mechanism needs something to send events, while the polling
>> system does not.
> 
> My understanding is limited, but the diagram at
> https://nodejs.org/en/docs/guides/event-loop-timers-and-nexttick/
> seems to say something different: the loop detects events by
> polling. (Yes, OK, that's talking about Javascript but the
> fundamentals are the same, surely?)

Following your comments and Guangpeng's we will try to considerably improve
and expand the text about this.

> 
>> section 2.3.1.3:
>> If you are going to insert C structures, why not use a real example?
>>   https://github.com/PJK/libcbor/blob/master/src/cbor/data.h#L164
>>   cbor_value -> cbor_item_t
> 
> Yes, we can look at that. There are various CBOR libraries in C.

Actually it looks as if the type we need is cbor_mutable_data
(not a single item), sinc the GRASP "value" can be an arbitrary
data structure.

>  
>> I don't understand what the asa_nonce is for.
> 
> OK, then we need more text.
> 
>> Is this something that the underlying GRASP library is supposed to use
>> internally to sort out which ASA is which?  Maybe this should be
>> either a UUID, or an opaque type, which might in some cases, be a file
>> handle, or contain one. (a la FILE *)
> 
> Yes, it should probably be opaque. In fact the documentation of the
> concrete Python API says so: "Note - the ASA must store the asa_nonce
> (an opaque Python object) and use it in every subsequent GRASP call."

Again, we will improve the text.

> 
>>
>> 2.3.3 Discovery.
>>   1) I would prefer to be able to ask for the list of cached
>>      locators for an objective directly.
> 
> OK, I can see that might be useful.

In fact, in the Python implementation, that's what you get from discover()
if the list is non-empty. Maybe we should change the meaning of timeout = 0
to mean what it says: simply return the list of cached locators as-is,
without waiting for discovery. I quite like that idea, since using
GRASP_DEF_TIMEOUT as the default for discovery timeouts was an abitrary
choice. Probably the ASA programmer should be forced to think about a
meaningful timeout.

> 
>>   2) Rather than flush them explicitely (because there might be
>>      other ASAs depending upon them), I'd like to do a discovery
>>      that asks for objectives that are at at most X miliseconds old.
>>      Flush is therefore X=0.
> 
> Hmm. I think we assumed you would only flush if you had reason to
> believe they were stale. But certainly adding an age limit seems
> reasonable.

Will do that - replace the flush parameter with age_limit.

>>   3) In both threaded and event-loop situations, I'd like to be able
>>      have a function called when there are new answers, otherwise,
>>      I have to poll.
> 
> Right, that's a callback in event loop terminology. It's a matter of
> preference; I have an aversion to side effects so I don't like callbacks.
> However, you're probably right that it should be an option.

This will be explicit in the next version.
 
>>      (And, if a function is called, then the question as to which
>>      thread does the calling is important; specifically one needs
>>      to know which other functions one call at that point
>>      answer may well be none. The answer depends upon how locking
>>      of structures is done. The answer is easier in event-loop
>>      version)
> 
> Yes; callbacks are a Bad Idea in a multithreaded version.

An ASA is responsible for the atomicity of its own transactions;
that's actually why I've been asserting that the session_nonce is
essential for callbacks. To be clear, GRASP negotiation messages
are *not* idempotent. So you can start several parallel negotiations
and they must be possible to distinguish from each other.
  
>> 2.3.4 Negotiation.
>>       Since the session_nonce is returned by the function, how
>>       can the dryrun/run ever be mixed in a single session?  maybe
>>       I don't know what a session is.
>>       Ah, it's a value/result parameters.   So, please put it into
>>       the input section too.
> 
> OK will think about that.

An ASA that makes use of dry run isn't quite straightforward. I never
tried writing a demo of dry run, except for a trivial test that the bit
gets transmitted correctly. There's presumably implied state that a
resource is pre-booked but not actually assigned. I think we need to
make the API agnostic about the semantics of dry run.

> 
>>
>> listen_negotiate.
>>       I think that this call is wrong in both threaded and event-loop
>>       use.  I think that in threaded version, I really want a new
>>       thread spawned off that does the right things (negotiate_wait, etc.),
> 
> You do need a new thread. But I don't have time right now to explain
> how that works in my example use cases. And my explanation would
> be Pythonesque.

Yes, there's no free lunch - it's the ASA writer's job to spawn a new thread
for a new transaction, in the threaded model. And in the event loop model,
the ASA needs to add a new event in the event loop for each new transaction.
We need to say this explicitly.

(It's no different than writing a server that listens for simultaneous
incoming TCP sessions; of course that's exactly what the GRASP core has
to do.)

> 
>>       so I want to provide a function for that thread to run the negotiation.
>>       In event-loop, I think that I want the same thing, but in the
>>       function, I can't do synchronous calls, so the function has to
>>       be called each time.
> 
> Sure. You have to insert a new event in the loop for each new negotiation.
> 
>> negotiate_step:
>>       It says:
>>          Threaded implementation: Called in the same thread as
>>          the preceding 'request_negotiate' or 'listen_negotiate', with the
>>          same value of 'session_nonce'.
>>       but if it's in the same thread as listen_negotiate(), then I can
>>       only handle negotiation with a single peer in that thread, I think?
> 
> Yes. Threads galore. A new thread for each new negotiation.
> 
>>       and I don't think I'd want to call listen_negotiate() for the
>>       same objective.
> 
> Actually that will depend on the use case, but if you do so, you need
> locks and atomicity. I put all of that into the prefix assignment demo,
> I think.
> https://github.com/becarpenter/graspy/blob/master/pfxm3.py 

When you think about it, there's nothing special about mixing 'request_negotiate'
and 'listen_negotiate'. The problem of atomicity is identical if you run
two 'request_negotiate' sessions or one of each. Whatever shared data structure
is involved must be used by one thread at a time.

>> negotiate_wait: no idea why I'd use this.
> 
> That simply refelects a GRASP feature: tell the other end to wait longer.
>>
>> Summary: I do not object to this document going forward as Informational, as
>>          it represents an explanation of one implementation, and that is
>>          what Informational is about.
> 
> All the same, I'd like to capture as much generality as possible.
> 
>>          It would be nice to have the view of multiple implementators, but
>>          there is no energy for that.
> 
> Yes, that's what we need.
> 
> Regards
>      Brian
> 
>>          While I do not object, I do not see great utility in this document.
>>
>> nits:
>>   2.3.1.3: s/neg/negotiate/
>>   I found the "NEG" term in GRASP confusing, because it seems like
>>   NEGative, rather then NEGotiate.  I'd prefer it was spelt out in
>>   the API.
>>   s/dry/dryrun/
>>
>>
>>
>>
>>
>> --
>> Michael Richardson <[e-mail]>, Sandelman Software Works
>>  -= IPv6 IoT consulting =-
>>
>>
>>
>>
>> _______________________________________________
>> Anima mailing list
>> [e-mail]
>> https://www.ietf.org/mailman/listinfo/anima
>>