Add details to the Framer Interface #323
Conversation
I made some minor requests. Why I don't want to require this to be part of this PR, I do think that we've reached a point with framers where we really need a simple example - more concretely than the "example" paragraph that talks about how to parse a header length field, in the form of Pseudocode. E.g., an application with TLV encoding perhaps?
draft-ietf-taps-interface.md
Outdated
|
|
||
| ## Defining Message Framers | ||
|
|
||
| Applications can define a Message Framer by creating a object to represent a unique |
There was a problem hiding this comment.
First, a nit: this should be "an object".
Second, more importantly: this is now written for object-oriented languages. I think there's no good way to write text about framers without deciding either to talk about objects or to talk about callbacks... so my proposal would be to preface this sentence with a statement like the following:
We describe Message Framers assuming an object-oriented programming language. In the case of programming languages that are not object-oriented, a Message Framer can be thought of as a number of functions and static data that they operate on. Adding a Framer to a MessageContext, as below, would then mean to register a callback to a function of the framer.
There was a problem hiding this comment.
@mwelzl: This is true for most of our API description and not specific to framers.
I would not add this kind of hit here, but maybe at a more general place.
There was a problem hiding this comment.
Thanks for the typo.
Mainly, I'm using object since we are using object in the rest of the interface description. Even for a non-object oriented language, you can still refer to an allocated structure with defined functions to use as an "object". A Connection is an object, etc.
There was a problem hiding this comment.
@fpauly I agree that there is some OO flavor through the whole document anyway. So maybe it's fine. I agree to not do anything about this now - and I agree with @philsbln that this is a more general thing and might better be written at a more general place, if we end up writing such a thing at all.
Conclusion: for this PR, I agree to leave it as it is.
I like the general representation of framers and we should definitely in that direction.
However, I am a bit puzzled with regards on the interactions between the objects. It looks somehow mixed up and either needs fixing or explanation.
In addition, I have the feeling we should give a hint what kind of data to put in the message context and what in the message object. At the moment, one could pass NULL messages and add all bits and pieces to the message context.
draft-ietf-taps-interface.md
Outdated
|
|
||
| ## Defining Message Framers | ||
|
|
||
| Applications can define a Message Framer by creating a object to represent a unique |
There was a problem hiding this comment.
@mwelzl: This is true for most of our API description and not specific to framers.
I would not add this kind of hit here, but maybe at a more general place.
|
|
||
| ~~~ | ||
| messageContext := NewMessageContext() | ||
| messageContext.add(framer, key, value) |
There was a problem hiding this comment.
This syntax overlaps with the syntax for message properties – while this is very elegant, it might add a lot of confusion for languages without polymorphism.
There was a problem hiding this comment.
Sure, in another language, you could have it be addFramerMessageValue or something.
| Upon receiving this event, a Message Framer implementation is responsible for | ||
| performing any necessary transformations and sending the resulting data to the next | ||
| protocol. Implementations SHOULD ensure that there is a way to pass the original data | ||
| through without copying to improve performance. |
There was a problem hiding this comment.
While this is a valid comment, is this really something that belongs in the API?
There was a problem hiding this comment.
This is based on the discussion at IETF 104 that indicated we wanted to ensure that the API allows for 0-copy. That is an API semantic decision.
draft-ietf-taps-interface.md
Outdated
| is first notified whenever new data is available to parse. | ||
|
|
||
| ~~~ | ||
| FramerInstance -> HandleReceivedData<> |
There was a problem hiding this comment.
This example broken:
- FramerInstance is the receiver of the event, not the sender
- We have no other events with "handle" in the name
- Arguments (Data + Context are missing)
There was a problem hiding this comment.
No, FramerInstance is not the receiver of the event, it is the sender. It's the handle to the representation of the framer within the Connection that you interact with. It's your state. It says: your framer now has data available.
The data for new inbound data should not be provided in this event—the implementation of the framer needs to parse the data out. The data may not be in a form that is efficiently consumable as a single data object (it's an entire stream).
There was a problem hiding this comment.
Okay - I got it. Also makes sense not to specify who receives this Event, as it could be another Framer or the connection object to forward it to the application.
Can we try to make this event match the Received/ReceivedPartial event somehow?
This would look much more consistent and allows to directly send it from the Framer.
There was a problem hiding this comment.
Maybe consider adding a railroad diagram to better illustrate the flow?
draft-ietf-taps-interface.md
Outdated
| If the data is not available, the parse fails. | ||
|
|
||
| ~~~ | ||
| instance.parse(minIncompleteLength, maxLength) -> (Data?, endOfMessage?) |
There was a problem hiding this comment.
why is this a call on the framer, and not on the connection?
There was a problem hiding this comment.
Because this is what the framer implementation does to read out the bytes. You can't call it on the top of a connection—that's for the application. There can be multiple framers in a connection, and you want to be able to parse the inbound data available for your instance of a framer.
There was a problem hiding this comment.
As with the HandleReceivedData<>, making it analogous to Received/ReceivedPartial would cause less confusion here
|
@mwelzl I agree that we should have an example somewhere, but I also thought this should be a different PR. |
I agree with Philipp in that the general direction is good but I am also very confused on why some things are events and who fires them, why the application appears to set up eventhandlers for the framer and the general interaction between the connection, application and framer.
draft-ietf-taps-interface.md
Outdated
|
|
||
| ~~~ | ||
| FramerInstance -> Start() | ||
| FramerInstance -> Stop() |
There was a problem hiding this comment.
I am confused by this part, if FramerInstance, i.e. the framer object created by the application, is the sender of the event, who is the receiver? How does the framer learn of a new connection?
It might just be me not understanding the notation correctly because in the text you do say that the framer receives the event.
If FramerInstance is not the same as framer, maybe it would be good to clarify what it actually is.
There was a problem hiding this comment.
FramerInstance is not created by the application. It is created by the connection, as the handle to the instance of the framer. The MessageFramer object is the definition created by the application.
There was a problem hiding this comment.
Why this? This deprives the application of the ability to pass arguments to the Framer's constructor.
There was a problem hiding this comment.
How much of this is essential to the interface, and how much of it is particular to a given implementation approach? Exposing a thing called FramerInstance is a yellow flag to me here...
draft-ietf-taps-interface.md
Outdated
|
|
||
| ~~~ | ||
| framer := NewMessageFramer() | ||
| framer.setEventHandlers() |
There was a problem hiding this comment.
I am unsure what the point of these event handlers is, is the framer supposed to directly interact with the application without going through the connection?
There was a problem hiding this comment.
You need something to add to the connection. The MessageFramer is the object that the application creates to do this.
There was a problem hiding this comment.
Oh I finally I think I understand where my confusion comes from. I assumed that the application was free to create their own framer class that had to expose certain functions for interaction with the connection, so I was looking for that interface. Sorry, my bad.
The place where the application now implements the behavior of framer is in the eventHandlers it sets, is that correct?
There was a problem hiding this comment.
so I think that @MaxF12 is on to something here. The generalized interface should be simpler and allow idiomatic implementation. In most languages (I'm familiar with) the idiomatic way to use an interface would be something like:
(1) application implements a framer or grabs an implementation off the shelf, and makes this framer available to the preconnection/listener.
(2) taps maintains a framer context for each connection, where the contents of the framer context are framer specific.
In idiomatic Java, for instance, the implementation would be a TransportServicesConnectionFramerFactory which would spit out TransportServicesConnectionFramers. In C, the framer is a struct of function pointers each of which takes a context. In a CSP patterned Go implementation, it's a struct with funcs reading messages/bytechunks from channels, each connection of which gets those funcs running as goroutines. And so on.
This arrangement seems to make these sorts of idioms more difficult.
There was a problem hiding this comment.
Thank you Brian, thats exactly what I couldn't quite put my finger on.
The previous iteration of framers made it very simple for applications that had no interest in implementing them to grab a framer implementation someone else provided and use it. All the calls and callbacks between the implementation and the TAPS system stayed the same, independent of whether or not a framer was used. I think its important to keep the part of the interface application developers use simple while adding complexity in the part someone that implements a framer would use, because at least to me these are two different groups of people.
There was a problem hiding this comment.
Agreed. The code here is just how you implement a custom framer. The calls to actually add it to a connection (the common action) is below.
There was a problem hiding this comment.
Of course, my bad. I totally missed that part, that makes things a lot clearer and I now understand the advantage of doing it this way around, thanks.
On a different note, why is the framer explicitly exposing a setEventHandlers call while neither the preconnection nor the connection expose something similar even though they issue events as well?
draft-ietf-taps-interface.md
Outdated
| ~~~ | ||
|
|
||
| When Message Framer gets a start message, it sets up its state and then indicates | ||
| to the connection that it is ready to handle sending Messages by calling `ready` on |
There was a problem hiding this comment.
If the framer calls 'ready', what does the interface with the connection look like? Why is this not an event that gets fired but rather a callable?
Same questions for instance.failed(Error).
There was a problem hiding this comment.
This is the event that the application's implementation of the framer sends to influence the main ready callbacks on the connection itself. It causes the event on the connection.
|
|
||
| Upon receiving this event, a Message Framer implementation is responsible for | ||
| performing any necessary transformations and sending the resulting data to the next | ||
| protocol. Implementations SHOULD ensure that there is a way to pass the original data |
There was a problem hiding this comment.
What happens if the final protocol has been reached? How does the message that went through all layers of framers get returned to the connection so it can be send out?
There was a problem hiding this comment.
The framers are part of the connection protocol stack.. they go down to the transport next.
draft-ietf-taps-interface.md
Outdated
| through without copying to improve performance. | ||
|
|
||
| ~~~ | ||
| instance.send(Data) |
There was a problem hiding this comment.
What exactly is this call supposed to do? Does it actually send out the message? If it does, why should the framer and not the connection be responsible for sending now?
There was a problem hiding this comment.
It sends data down to the next protocol down in the stack. The application sends original data on the connection, it gets sent to the first framer, which sends it to the next, which eventually sends it to the transport, which sends it out the device.
There was a problem hiding this comment.
Firing a Send-Event to the next instance would look much more consistent here…
| instance.advanceReceiveCursor(length) | ||
| instance.deliverAndAdvanceReceiveCursor(messageContext, length, endOfMessage) | ||
| instance.deliver(messageContext, data, endOfMessage) | ||
| ~~~ |
There was a problem hiding this comment.
This is another point where I am unsure on how the framer actually interfaces with the connection. Who calls instance.advanceReceiveCursor?
There was a problem hiding this comment.
The application's implementation of the framer.
|
One comment/request: I would assume that most framers would have a length field. I wonder if it would be make sense to provide an interface to indicate where to find the length and move the parsing task into the transport. That would implementing simple framers that only have a length super easy... |
Thanks for the text and starting the discussion!
Two high-level comments:
(1) This seems to have a particular implementation too specifically in mind. Please generalize.
(2) Pictures and examples would help, too.
(Will re-review after the rest of the raft of comments have been addressed.)
draft-ietf-taps-interface.md
Outdated
|
|
||
| ~~~ | ||
| FramerInstance -> Start() | ||
| FramerInstance -> Stop() |
There was a problem hiding this comment.
How much of this is essential to the interface, and how much of it is particular to a given implementation approach? Exposing a thing called FramerInstance is a yellow flag to me here...
draft-ietf-taps-interface.md
Outdated
|
|
||
| ~~~ | ||
| framer := NewMessageFramer() | ||
| framer.setEventHandlers() |
There was a problem hiding this comment.
so I think that @MaxF12 is on to something here. The generalized interface should be simpler and allow idiomatic implementation. In most languages (I'm familiar with) the idiomatic way to use an interface would be something like:
(1) application implements a framer or grabs an implementation off the shelf, and makes this framer available to the preconnection/listener.
(2) taps maintains a framer context for each connection, where the contents of the framer context are framer specific.
In idiomatic Java, for instance, the implementation would be a TransportServicesConnectionFramerFactory which would spit out TransportServicesConnectionFramers. In C, the framer is a struct of function pointers each of which takes a context. In a CSP patterned Go implementation, it's a struct with funcs reading messages/bytechunks from channels, each connection of which gets those funcs running as goroutines. And so on.
This arrangement seems to make these sorts of idioms more difficult.
|
Not done, but did some cleanup to remove the Instance object, etc. Now there's just a MessageFramer that interacts with the Connection, which simplifies things a bunch. I'd like to add some diagrams, but also a code example of a simple length-value header framer. I was looking at our "sample" code for a client and server, and it's not clear to me in our syntax how we show what the code does upon receiving an event. Let's say, I want to show that my message framer is called to handle a new sent message. Where do I write that "block" of code? @britram @mwelzl ? |
|
Earlier when we discussed an example, we agreed that we need it but that we could leave it out of this PR. Another answer: I think the receiving code is also changed by PR #332. ISTM that landing this one as well as #332 first, and then writing this example code would be a good sequence. UPDATE: I just noticed that you approved PR #332 three minutes ago, so I just clicked the "merge" button. I hope that this makes things easier. |
Thank you for making the Interface clearer and easier.
I am still a little confused by the interaction pattern and what a Framer object really is.
- Is a Framer object something provided by the Transport System that tracks state of the de-framing process, like a Cursor or
- Is a Framer an object provided by the application or a library that implements the Framer protocol.
At the moment, the beginning of the texts suggests the latter while the interaction pattern suggest the former.
| ~~~ | ||
| Preconnection.AddFramer(framer) | ||
| ~~~ |
There was a problem hiding this comment.
Maybe add a forward reference how to prependFramer here
|
|
||
| ~~~ | ||
| messageContext := NewMessageContext() | ||
| messageContext.add(framer, key, value) |
| ~~~ | ||
| MessageFramer -> Start(Connection) | ||
| MessageFramer -> Stop(Connection) | ||
| ~~~ |
There was a problem hiding this comment.
This Syntax is inconsistent. It says the MessageFramer emits a Start/Stop Event, but the text says it consumes it.
There was a problem hiding this comment.
Clarified event delivery
| ~~~ | ||
| otherFramer := NewMessageFramer() | ||
| MessageFramer.PrependFramer(Connection, otherFramer) | ||
| ~~~ |
There was a problem hiding this comment.
Do we only want to support prepend here or should we also allow to append framers here?
There was a problem hiding this comment.
I think, since handshakes build from the bottom up, and you don't want outstanding data in the stream below you, it's only safe to prepend.
There was a problem hiding this comment.
Right. So if a would do some kind of multiplexing within my framer, i can not easily reuse other framers that further process my messages. Anyway, this would require to dispatch stuff to another framer, not appending it.
draft-ietf-taps-interface.md
Outdated
|
|
||
| ## Sender-side Message Framing {#send-framing} | ||
|
|
||
| Message Framers deliver an event whenever a Connection sends a new Message. |
There was a problem hiding this comment.
Clarified event delivery
| ~~~ | ||
| MessageFramer.Parse(Connection, MinimumIncompleteLength, MaximumLength) -> (Data?, IsEndOfMessage?) | ||
| ~~~ | ||
|
|
There was a problem hiding this comment.
This implies a Framer would always operate on a byte stream / partial message.
Should we add an Interface to de-frame data that is already a Message?
There was a problem hiding this comment.
Hm, so it should be able to handle messages fine, since this is handling partial messages. I think the most common case of framer is over a bye stream, so I think handling that by default is important. But if you have a high MinimumIncompleteLength, any shorter Message will just be delivered along with IsEndOfMessage.
I suppose the thing to add here is the Message object itself.
There was a problem hiding this comment.
Added MessageContext here.
|
@philsbln thanks for the comments! I added more clarification that there's a Message Framer object that delivers events to the framer implementation code, and made it more clear that this "framer implementation" code is something that the application or some other library does, and it's what receives the events to actual do the framing and run its code. |
Thank you @tfpauly for clarifying - now it is clear what a framer is.
I still don't like the the design of a framer being an object provided by the transport system that takes care of the buffer management, while the actual framing is done in in callbacks interacting with it.
While this is a very reasonable design to minimise copying data, it really feels wrong from an OO perspective.
Despite this dislike, I think we should land this PR now and discuss the design in Montreal.
|
So sorry for leaving this dangling! I looked at this PR recently but was too clumsy to notice that my requests have long been addressed :( now, I'd be fine with merging this, but again I'm afraid of my clumsiness because this talks of conflicts... my suggestion: give @britram a day or two too, and then just go ahead and merge this in yourself. Either way, I think it's easy to agree that we should merge this now, and if there would be small fixes needed they can still be done later. |
|
Okay, based on @mwelzl's request, merging in now. Please file new issues for further changes! |
This PR is based on the presentation given at IETF 104 and the received feedback regarding Framers. This enhances and clarifies the API text around how framers are set up and used.