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 <[email protected]>, Sandelman Software Works >> -= IPv6 IoT consulting =- >> >> >> >> >> _______________________________________________ >> Anima mailing list >> [email protected] >> https://www.ietf.org/mailman/listinfo/anima >> _______________________________________________ Anima mailing list [email protected] https://www.ietf.org/mailman/listinfo/anima
