Re: transport interface changes
I guess the value of the metaphor comes down to how useful it is in explaining how things work. I've put together a little intro that uses the metaphor to explain what a protocol engine is: . https://cwiki.apache.org/confluence/display/qpid/Protocol%20Engines It was very much inspired by my attempt to reply to this email and explain why I though the circular buffer/byte queue thing was a powerful tool for explaining things. I don't know if it influences your feelings on the subject, but hopefully it helps explain the model in my head and why I tend to think of the transport interface the way I do. My preference would be for maintaining the strong analogy between transport and byte queue/circular buffer. I like the fact that it emphasizes that the engine is at it's heart a pure data structure and not some kind of complex processing pipeline that might do work behind the scenes in other threads, something that is fairly critical to understanding how to use the engine. That said if the majority feel the input/output prefixes are helpful and don't detract from the value of these explanations then I can certainly live with them. --Rafael On Thu, Feb 7, 2013 at 11:43 AM, Phil Harvey p...@philharveyonline.comwrote: My 2 cents on the naming issue: I'm not convinced that a single queue is the best metaphor for the Transport, even if qualified by the term transforming. The meaning of the input and output data is surely so different that calling it a queue masks the essence of what the engine does. To me, a transforming queue suggests something that spits out something semantically identical to its input. For example, a byte queue whose head is a UTF-8-encoded transformation of its UTF-8 tail. I don't think Transport falls into this category, therefore my preference would be for the words input and output to appear in the function names. Phil On 7 February 2013 14:23, Ken Giusti kgiu...@redhat.com wrote: What we've got here is failure to communicate. There aren't necessarily distinct input/output buffer objects, rather the whole transport interface itself is really just single structure (a [transforming] byte queue) with push/peek/pop corresponding exactly to any standard queue interface. Aha! Well, that explains it - I've always though that the transport was composed of two separate buffers - one for input, the other for output. At least, that's my interpretation of the existing API. A transforming byte queue didn't immediately pop into my mind when reading these new APIs. You may want to add a bit of documentation to that patch explaining this meme before the APIs are described. Would be quite useful to anyone attempting to implement a driver. -K - Original Message - Looks like the attachement didn't make it. Here's the link to the patch on JIRA: https://issues.apache.org/jira/secure/attachment/12568408/transport.patch --Rafael On Thu, Feb 7, 2013 at 8:10 AM, Rafael Schloming r...@alum.mit.edu wrote: Here's a patch to engine.h with updated docs/naming. I've documented what I believe would be future lifecycle semantics, however as I said before I think an initial patch would need to be more conservative. I think these names would also work with input/output prefixes, although as the interface now pretty much exactly matches a circular buffer (i.e. a byte queue), I find the input/output prefixes a bit jarring. --Rafael On Thu, Feb 7, 2013 at 5:53 AM, Rafael Schloming r...@alum.mit.edu wrote: On Wed, Feb 6, 2013 at 5:12 PM, Rafael Schloming r...@alum.mit.eduwrote: On Wed, Feb 6, 2013 at 2:13 PM, Ken Giusti kgiu...@redhat.com wrote: Rafi, I agree with the rational behind these changes. /** Return a pointer to a transport's input buffer. This pointer may * change when calls into the engine are made. I think we'll need to be a little more liberal with the lifecycle guarantee of these buffer pointers. Drivers based on completion models (rather than sockets) could be forced to do data copies rather than supplying the pointer directly to the completion mechanism. That sentence was actually supposed to be deleted. The sentences after that describes the intended lifecycle policy for the input buffer: Calls to ::pn_transport_push may change the value of this pointer and the amount of free space reported by ::pn_transport_capacity. Could we instead guarantee that pointers (and space) returned from the transport remain valid until 1) the transport is released or 2) the push/pop call is made against the transport? That is in fact what I intended for push. For pop this would place a lot more restrictions on the engine implementation. Say for
Re: transport interface changes
FYI I've raised PROTON-225 to cover the API redesign so that we have the option of implementing it separately from the PROTON-222 bug fix. On 7 February 2013 16:43, Phil Harvey p...@philharveyonline.com wrote: My 2 cents on the naming issue: I'm not convinced that a single queue is the best metaphor for the Transport, even if qualified by the term transforming. The meaning of the input and output data is surely so different that calling it a queue masks the essence of what the engine does. To me, a transforming queue suggests something that spits out something semantically identical to its input. For example, a byte queue whose head is a UTF-8-encoded transformation of its UTF-8 tail. I don't think Transport falls into this category, therefore my preference would be for the words input and output to appear in the function names. Phil On 7 February 2013 14:23, Ken Giusti kgiu...@redhat.com wrote: What we've got here is failure to communicate. There aren't necessarily distinct input/output buffer objects, rather the whole transport interface itself is really just single structure (a [transforming] byte queue) with push/peek/pop corresponding exactly to any standard queue interface. Aha! Well, that explains it - I've always though that the transport was composed of two separate buffers - one for input, the other for output. At least, that's my interpretation of the existing API. A transforming byte queue didn't immediately pop into my mind when reading these new APIs. You may want to add a bit of documentation to that patch explaining this meme before the APIs are described. Would be quite useful to anyone attempting to implement a driver. -K - Original Message - Looks like the attachement didn't make it. Here's the link to the patch on JIRA: https://issues.apache.org/jira/secure/attachment/12568408/transport.patch --Rafael On Thu, Feb 7, 2013 at 8:10 AM, Rafael Schloming r...@alum.mit.edu wrote: Here's a patch to engine.h with updated docs/naming. I've documented what I believe would be future lifecycle semantics, however as I said before I think an initial patch would need to be more conservative. I think these names would also work with input/output prefixes, although as the interface now pretty much exactly matches a circular buffer (i.e. a byte queue), I find the input/output prefixes a bit jarring. --Rafael On Thu, Feb 7, 2013 at 5:53 AM, Rafael Schloming r...@alum.mit.edu wrote: On Wed, Feb 6, 2013 at 5:12 PM, Rafael Schloming r...@alum.mit.eduwrote: On Wed, Feb 6, 2013 at 2:13 PM, Ken Giusti kgiu...@redhat.com wrote: Rafi, I agree with the rational behind these changes. /** Return a pointer to a transport's input buffer. This pointer may * change when calls into the engine are made. I think we'll need to be a little more liberal with the lifecycle guarantee of these buffer pointers. Drivers based on completion models (rather than sockets) could be forced to do data copies rather than supplying the pointer directly to the completion mechanism. That sentence was actually supposed to be deleted. The sentences after that describes the intended lifecycle policy for the input buffer: Calls to ::pn_transport_push may change the value of this pointer and the amount of free space reported by ::pn_transport_capacity. Could we instead guarantee that pointers (and space) returned from the transport remain valid until 1) the transport is released or 2) the push/pop call is made against the transport? That is in fact what I intended for push. For pop this would place a lot more restrictions on the engine implementation. Say for example peek is called and then the top half is used to write message data. Ideally there should actually be more data to write over the network, which means that the transport may want to grow (realloc) the output buffer, and this of course is more complex if the external pointer needs to stay valid. Given that at worst this will incur an extra copy that is equivalent to what we're currently doing, I figured it would be safer to start out with more conservative semantics. We can always relax them later when we have had more time to consider the implementation. Or perhaps a reference count-based interface would be safer? Once the driver determines there is capacity/pending, it reserves the buffer and is required to push/pop to release it? Oh, and those names absolutely stink. ;) It's unclear from the function names what components of the transport they are affecting. I'd rather something more readable: pn_transport_capacity() -- pn_transport_input_capacity() pn_transport_buffer() -
Re: transport interface changes
On Wed, Feb 6, 2013 at 2:13 PM, Ken Giusti kgiu...@redhat.com wrote: Rafi, I agree with the rational behind these changes. /** Return a pointer to a transport's input buffer. This pointer may * change when calls into the engine are made. I think we'll need to be a little more liberal with the lifecycle guarantee of these buffer pointers. Drivers based on completion models (rather than sockets) could be forced to do data copies rather than supplying the pointer directly to the completion mechanism. Could we instead guarantee that pointers (and space) returned from the transport remain valid until 1) the transport is released or 2) the push/pop call is made against the transport? Or perhaps a reference count-based interface would be safer? Once the driver determines there is capacity/pending, it reserves the buffer and is required to push/pop to release it? I think I may have just grokked what you meant by a reference count based interface. If you're saying that whenever the transport reports pending output or available capacity it will preserve any exposed pointers until after that amount of capacity/pending bytes have been pushed/popped respectively, then I think that's a good semantic to shoot for. I wouldn't call it ref counting as I think we could track it more simply than that. I'd still say we should start with the more conservative semantics to get a patch in place to fix the bug, and then narrow them to this in a subsequent patch. --Rafael
Re: transport interface changes
On Wed, Feb 6, 2013 at 5:12 PM, Rafael Schloming r...@alum.mit.edu wrote: On Wed, Feb 6, 2013 at 2:13 PM, Ken Giusti kgiu...@redhat.com wrote: Rafi, I agree with the rational behind these changes. /** Return a pointer to a transport's input buffer. This pointer may * change when calls into the engine are made. I think we'll need to be a little more liberal with the lifecycle guarantee of these buffer pointers. Drivers based on completion models (rather than sockets) could be forced to do data copies rather than supplying the pointer directly to the completion mechanism. That sentence was actually supposed to be deleted. The sentences after that describes the intended lifecycle policy for the input buffer: Calls to ::pn_transport_push may change the value of this pointer and the amount of free space reported by ::pn_transport_capacity. Could we instead guarantee that pointers (and space) returned from the transport remain valid until 1) the transport is released or 2) the push/pop call is made against the transport? That is in fact what I intended for push. For pop this would place a lot more restrictions on the engine implementation. Say for example peek is called and then the top half is used to write message data. Ideally there should actually be more data to write over the network, which means that the transport may want to grow (realloc) the output buffer, and this of course is more complex if the external pointer needs to stay valid. Given that at worst this will incur an extra copy that is equivalent to what we're currently doing, I figured it would be safer to start out with more conservative semantics. We can always relax them later when we have had more time to consider the implementation. Or perhaps a reference count-based interface would be safer? Once the driver determines there is capacity/pending, it reserves the buffer and is required to push/pop to release it? Oh, and those names absolutely stink. ;) It's unclear from the function names what components of the transport they are affecting. I'd rather something more readable: pn_transport_capacity() -- pn_transport_input_capacity() pn_transport_buffer() - pn_transport_input_buffer() pn_transport_push() -- pn_transport_input_written() I think your names (and my documentation) actually suffer from exposing too much of the implementation. There aren't necessarily distinct input/output buffer objects, rather the whole transport interface itself is really just single structure (a [transforming] byte queue) with push/peek/pop corresponding exactly to any standard queue interface. FWIW, I do agree pn_transport_buffer() could be better, however capacity, pending, push, peek, and pop all correspond to pretty much every standard queue/circular buffer/stack interface I've ever seen, and my hasty documentation notwithstanding, I think that is a good meme to take advantage of, particularly since the push/peek/pop semantics and their relationship to each other is actually quite important in guaranteeing that the pitfall occurring in PROTON-222 doesn't come up when people code their own drivers. To elaborate on this a little, I think the key part of the meme that I like is how peek strongly signals non-destructive-read and pop strongly signals a destructive operation. I don't get that same destructive operation sense from output_written, and I think particularly when considering the lifecycle semantics, the non-destructive/destructive nature of the operations is kind of key. FWIW, I can buy the input/output prefix as it's possibly equally valid to view the interface as a queue head and a queue tail that aren't on the same queue, however I would personally omit them for two reasons (1) the notion of a transforming queue signals to the user that when you provide input, you should expect there to be output, and (2) I appreciate brevity. ;-) --Rafael
Re: transport interface changes
Here's a patch to engine.h with updated docs/naming. I've documented what I believe would be future lifecycle semantics, however as I said before I think an initial patch would need to be more conservative. I think these names would also work with input/output prefixes, although as the interface now pretty much exactly matches a circular buffer (i.e. a byte queue), I find the input/output prefixes a bit jarring. --Rafael On Thu, Feb 7, 2013 at 5:53 AM, Rafael Schloming r...@alum.mit.edu wrote: On Wed, Feb 6, 2013 at 5:12 PM, Rafael Schloming r...@alum.mit.edu wrote: On Wed, Feb 6, 2013 at 2:13 PM, Ken Giusti kgiu...@redhat.com wrote: Rafi, I agree with the rational behind these changes. /** Return a pointer to a transport's input buffer. This pointer may * change when calls into the engine are made. I think we'll need to be a little more liberal with the lifecycle guarantee of these buffer pointers. Drivers based on completion models (rather than sockets) could be forced to do data copies rather than supplying the pointer directly to the completion mechanism. That sentence was actually supposed to be deleted. The sentences after that describes the intended lifecycle policy for the input buffer: Calls to ::pn_transport_push may change the value of this pointer and the amount of free space reported by ::pn_transport_capacity. Could we instead guarantee that pointers (and space) returned from the transport remain valid until 1) the transport is released or 2) the push/pop call is made against the transport? That is in fact what I intended for push. For pop this would place a lot more restrictions on the engine implementation. Say for example peek is called and then the top half is used to write message data. Ideally there should actually be more data to write over the network, which means that the transport may want to grow (realloc) the output buffer, and this of course is more complex if the external pointer needs to stay valid. Given that at worst this will incur an extra copy that is equivalent to what we're currently doing, I figured it would be safer to start out with more conservative semantics. We can always relax them later when we have had more time to consider the implementation. Or perhaps a reference count-based interface would be safer? Once the driver determines there is capacity/pending, it reserves the buffer and is required to push/pop to release it? Oh, and those names absolutely stink. ;) It's unclear from the function names what components of the transport they are affecting. I'd rather something more readable: pn_transport_capacity() -- pn_transport_input_capacity() pn_transport_buffer() - pn_transport_input_buffer() pn_transport_push() -- pn_transport_input_written() I think your names (and my documentation) actually suffer from exposing too much of the implementation. There aren't necessarily distinct input/output buffer objects, rather the whole transport interface itself is really just single structure (a [transforming] byte queue) with push/peek/pop corresponding exactly to any standard queue interface. FWIW, I do agree pn_transport_buffer() could be better, however capacity, pending, push, peek, and pop all correspond to pretty much every standard queue/circular buffer/stack interface I've ever seen, and my hasty documentation notwithstanding, I think that is a good meme to take advantage of, particularly since the push/peek/pop semantics and their relationship to each other is actually quite important in guaranteeing that the pitfall occurring in PROTON-222 doesn't come up when people code their own drivers. To elaborate on this a little, I think the key part of the meme that I like is how peek strongly signals non-destructive-read and pop strongly signals a destructive operation. I don't get that same destructive operation sense from output_written, and I think particularly when considering the lifecycle semantics, the non-destructive/destructive nature of the operations is kind of key. FWIW, I can buy the input/output prefix as it's possibly equally valid to view the interface as a queue head and a queue tail that aren't on the same queue, however I would personally omit them for two reasons (1) the notion of a transforming queue signals to the user that when you provide input, you should expect there to be output, and (2) I appreciate brevity. ;-) --Rafael
Re: transport interface changes
Looks like the attachement didn't make it. Here's the link to the patch on JIRA: https://issues.apache.org/jira/secure/attachment/12568408/transport.patch --Rafael On Thu, Feb 7, 2013 at 8:10 AM, Rafael Schloming r...@alum.mit.edu wrote: Here's a patch to engine.h with updated docs/naming. I've documented what I believe would be future lifecycle semantics, however as I said before I think an initial patch would need to be more conservative. I think these names would also work with input/output prefixes, although as the interface now pretty much exactly matches a circular buffer (i.e. a byte queue), I find the input/output prefixes a bit jarring. --Rafael On Thu, Feb 7, 2013 at 5:53 AM, Rafael Schloming r...@alum.mit.edu wrote: On Wed, Feb 6, 2013 at 5:12 PM, Rafael Schloming r...@alum.mit.eduwrote: On Wed, Feb 6, 2013 at 2:13 PM, Ken Giusti kgiu...@redhat.com wrote: Rafi, I agree with the rational behind these changes. /** Return a pointer to a transport's input buffer. This pointer may * change when calls into the engine are made. I think we'll need to be a little more liberal with the lifecycle guarantee of these buffer pointers. Drivers based on completion models (rather than sockets) could be forced to do data copies rather than supplying the pointer directly to the completion mechanism. That sentence was actually supposed to be deleted. The sentences after that describes the intended lifecycle policy for the input buffer: Calls to ::pn_transport_push may change the value of this pointer and the amount of free space reported by ::pn_transport_capacity. Could we instead guarantee that pointers (and space) returned from the transport remain valid until 1) the transport is released or 2) the push/pop call is made against the transport? That is in fact what I intended for push. For pop this would place a lot more restrictions on the engine implementation. Say for example peek is called and then the top half is used to write message data. Ideally there should actually be more data to write over the network, which means that the transport may want to grow (realloc) the output buffer, and this of course is more complex if the external pointer needs to stay valid. Given that at worst this will incur an extra copy that is equivalent to what we're currently doing, I figured it would be safer to start out with more conservative semantics. We can always relax them later when we have had more time to consider the implementation. Or perhaps a reference count-based interface would be safer? Once the driver determines there is capacity/pending, it reserves the buffer and is required to push/pop to release it? Oh, and those names absolutely stink. ;) It's unclear from the function names what components of the transport they are affecting. I'd rather something more readable: pn_transport_capacity() -- pn_transport_input_capacity() pn_transport_buffer() - pn_transport_input_buffer() pn_transport_push() -- pn_transport_input_written() I think your names (and my documentation) actually suffer from exposing too much of the implementation. There aren't necessarily distinct input/output buffer objects, rather the whole transport interface itself is really just single structure (a [transforming] byte queue) with push/peek/pop corresponding exactly to any standard queue interface. FWIW, I do agree pn_transport_buffer() could be better, however capacity, pending, push, peek, and pop all correspond to pretty much every standard queue/circular buffer/stack interface I've ever seen, and my hasty documentation notwithstanding, I think that is a good meme to take advantage of, particularly since the push/peek/pop semantics and their relationship to each other is actually quite important in guaranteeing that the pitfall occurring in PROTON-222 doesn't come up when people code their own drivers. To elaborate on this a little, I think the key part of the meme that I like is how peek strongly signals non-destructive-read and pop strongly signals a destructive operation. I don't get that same destructive operation sense from output_written, and I think particularly when considering the lifecycle semantics, the non-destructive/destructive nature of the operations is kind of key. FWIW, I can buy the input/output prefix as it's possibly equally valid to view the interface as a queue head and a queue tail that aren't on the same queue, however I would personally omit them for two reasons (1) the notion of a transforming queue signals to the user that when you provide input, you should expect there to be output, and (2) I appreciate brevity. ;-) --Rafael
Re: transport interface changes
Thanks for the clear description Rafi. The modified interface sounds
reasonable, notwithstanding the resolution of the questions about naming,
and of the valid lifespan of the input/output pointers.
We're currently looking at the impact on proton-j and will respond more
fully once we've got a better understanding of it.
I believe there has been some brief discussion off-list of how the
corresponding Java Transport API would look. This will be implemented both
in pure Java and via JNI calls to proton-c. I've summarised my second-hand
understanding of the proposed Java API below.
interface Transport
{
/**
* Like pn_transport_buffer.
* The ByteBuffer.remaining() method obviates the need for a
pn_transport_capacity equivalent
*/
ByteBuffer getInputBuffer();
/** like push. No need for corresponding size parameter because the
input buffer's position() implies it */
int processInput();
/** like pn_transport_peek */
ByteBuffer getOutputBuffer();
/** like pop */
int outputWritten();
/** ... deprecated existing input/output methods */
}
Phil
On 6 February 2013 17:14, Rafael Schloming r...@alum.mit.edu wrote:
A recent bug (PROTON-222) has exposed an issue with the transport
interface. The details of the bug are documented in the JIRA, but the basic
issue is that given the current transport interface there is no way for an
application to discover via the engine interface when data has been
actually written over the wire vs just sitting there waiting to be written
over the wire.
Note that by written over the wire I mean copied from an in-process
buffer that will vanish as soon as the process exits, to a kernel buffer
that will continue to (re)-transmit the data for as long as the machine is
alive, connected to the network, and the remote endpoint is still
listening.
To understand the issue, imagine implementing a driver for the current
transport interface. It might allocate a buffer of 1K and then call
pn_transport_output to fill that buffer. The transport might then put say
750 bytes of data into that buffer. Now imagine what happens if the driver
can only write 500 of those bytes to the socket. This will leave the driver
buffering 250 bytes. The engine of course has no way of knowing this and
can only assume that all 750 bytes will get written out.
A somewhat related issue is the buffering ownership/strategy between the
transport and the driver. There are really three basic choices here:
1) the driver owns the buffer, and the transport does no buffering
2) the transport owns the buffer, and the driver does no buffering
3) they both own their own buffers and we copy from one to the other
Now the division between these isn't always static, there are hybrid
strategies and what not, however it's useful to think of these basic cases.
The current transport interface (pn_transport_input/output) and initial
implementation was designed around option (1), the idea behind the
pn_transport_output signature was that the engine could directly encode
into a driver-owned buffer. This, however, turned out to introduce some
unfriendly coupling. Imagine what happens in our hypothetical scenario
above if the driver has a 1K buffer and the engine negotiates a 4K frame
size. The engine might end up getting stuck with a frame that is too large
to encode directly into the driver's buffer. So to make the interface more
friendly, we modified the implementation to do buffering internally if
necessary, thus ending up in some ways closer to option (3).
Now the reason this buffering issue is related to PROTON-222 is that one
way to allow the engine to know whether data is buffered or not is to
redefine the interface around option (2), thereby allowing the engine to
always have visibility into what is/isn't on the wire. This would also in
some cases eliminate some of the extra copying that occurs currently due to
our evolution towards option (3).
Such an interface would look something like this:
// deprecated and internally implemented with capacity, buffer, and
push
ssize_t pn_transport_input(pn_transport_t *transport, const char
*bytes, size_t available);
// deprecated and internally implemented with pending, peek, and pop
ssize_t pn_transport_output(pn_transport_t *transport, char *bytes,
size_t size);
/** Report the amount of free space in a transport's input buffer. If
* the engine is in an error state or has reached the end of stream
* state, a negative value will be returned indication the condition.
*
* @param[in] transport the transport
* @return the free space in the transport
*/
ssize_t pn_transport_capacity(pn_transport_t *transport);
/** Return a pointer to a transport's input buffer. This pointer may
* change when calls into the engine are made. The amount of space in
* this buffer is reported by ::pn_transport_capacity. Calls
Re: transport interface changes
What we've got here is failure to communicate. There aren't necessarily distinct input/output buffer objects, rather the whole transport interface itself is really just single structure (a [transforming] byte queue) with push/peek/pop corresponding exactly to any standard queue interface. Aha! Well, that explains it - I've always though that the transport was composed of two separate buffers - one for input, the other for output. At least, that's my interpretation of the existing API. A transforming byte queue didn't immediately pop into my mind when reading these new APIs. You may want to add a bit of documentation to that patch explaining this meme before the APIs are described. Would be quite useful to anyone attempting to implement a driver. -K - Original Message - Looks like the attachement didn't make it. Here's the link to the patch on JIRA: https://issues.apache.org/jira/secure/attachment/12568408/transport.patch --Rafael On Thu, Feb 7, 2013 at 8:10 AM, Rafael Schloming r...@alum.mit.edu wrote: Here's a patch to engine.h with updated docs/naming. I've documented what I believe would be future lifecycle semantics, however as I said before I think an initial patch would need to be more conservative. I think these names would also work with input/output prefixes, although as the interface now pretty much exactly matches a circular buffer (i.e. a byte queue), I find the input/output prefixes a bit jarring. --Rafael On Thu, Feb 7, 2013 at 5:53 AM, Rafael Schloming r...@alum.mit.edu wrote: On Wed, Feb 6, 2013 at 5:12 PM, Rafael Schloming r...@alum.mit.eduwrote: On Wed, Feb 6, 2013 at 2:13 PM, Ken Giusti kgiu...@redhat.com wrote: Rafi, I agree with the rational behind these changes. /** Return a pointer to a transport's input buffer. This pointer may * change when calls into the engine are made. I think we'll need to be a little more liberal with the lifecycle guarantee of these buffer pointers. Drivers based on completion models (rather than sockets) could be forced to do data copies rather than supplying the pointer directly to the completion mechanism. That sentence was actually supposed to be deleted. The sentences after that describes the intended lifecycle policy for the input buffer: Calls to ::pn_transport_push may change the value of this pointer and the amount of free space reported by ::pn_transport_capacity. Could we instead guarantee that pointers (and space) returned from the transport remain valid until 1) the transport is released or 2) the push/pop call is made against the transport? That is in fact what I intended for push. For pop this would place a lot more restrictions on the engine implementation. Say for example peek is called and then the top half is used to write message data. Ideally there should actually be more data to write over the network, which means that the transport may want to grow (realloc) the output buffer, and this of course is more complex if the external pointer needs to stay valid. Given that at worst this will incur an extra copy that is equivalent to what we're currently doing, I figured it would be safer to start out with more conservative semantics. We can always relax them later when we have had more time to consider the implementation. Or perhaps a reference count-based interface would be safer? Once the driver determines there is capacity/pending, it reserves the buffer and is required to push/pop to release it? Oh, and those names absolutely stink. ;) It's unclear from the function names what components of the transport they are affecting. I'd rather something more readable: pn_transport_capacity() -- pn_transport_input_capacity() pn_transport_buffer() - pn_transport_input_buffer() pn_transport_push() -- pn_transport_input_written() I think your names (and my documentation) actually suffer from exposing too much of the implementation. There aren't necessarily distinct input/output buffer objects, rather the whole transport interface itself is really just single structure (a [transforming] byte queue) with push/peek/pop corresponding exactly to any standard queue interface. FWIW, I do agree pn_transport_buffer() could be better, however capacity, pending, push, peek, and pop all correspond to pretty much every standard queue/circular buffer/stack interface I've ever seen, and my hasty documentation notwithstanding, I think that is a good meme to take advantage of, particularly since the push/peek/pop semantics and their relationship to each other is actually quite important in guaranteeing that the pitfall occurring in PROTON-222 doesn't come up when people code their own drivers. To elaborate on this a little, I think the key part of the meme
Re: transport interface changes
Oh, and +1 to the patch. The names are much better, and the descriptions make the use model much clearer. -K - Original Message - What we've got here is failure to communicate. There aren't necessarily distinct input/output buffer objects, rather the whole transport interface itself is really just single structure (a [transforming] byte queue) with push/peek/pop corresponding exactly to any standard queue interface. Aha! Well, that explains it - I've always though that the transport was composed of two separate buffers - one for input, the other for output. At least, that's my interpretation of the existing API. A transforming byte queue didn't immediately pop into my mind when reading these new APIs. You may want to add a bit of documentation to that patch explaining this meme before the APIs are described. Would be quite useful to anyone attempting to implement a driver. -K - Original Message - Looks like the attachement didn't make it. Here's the link to the patch on JIRA: https://issues.apache.org/jira/secure/attachment/12568408/transport.patch --Rafael On Thu, Feb 7, 2013 at 8:10 AM, Rafael Schloming r...@alum.mit.edu wrote: Here's a patch to engine.h with updated docs/naming. I've documented what I believe would be future lifecycle semantics, however as I said before I think an initial patch would need to be more conservative. I think these names would also work with input/output prefixes, although as the interface now pretty much exactly matches a circular buffer (i.e. a byte queue), I find the input/output prefixes a bit jarring. --Rafael On Thu, Feb 7, 2013 at 5:53 AM, Rafael Schloming r...@alum.mit.edu wrote: On Wed, Feb 6, 2013 at 5:12 PM, Rafael Schloming r...@alum.mit.eduwrote: On Wed, Feb 6, 2013 at 2:13 PM, Ken Giusti kgiu...@redhat.com wrote: Rafi, I agree with the rational behind these changes. /** Return a pointer to a transport's input buffer. This pointer may * change when calls into the engine are made. I think we'll need to be a little more liberal with the lifecycle guarantee of these buffer pointers. Drivers based on completion models (rather than sockets) could be forced to do data copies rather than supplying the pointer directly to the completion mechanism. That sentence was actually supposed to be deleted. The sentences after that describes the intended lifecycle policy for the input buffer: Calls to ::pn_transport_push may change the value of this pointer and the amount of free space reported by ::pn_transport_capacity. Could we instead guarantee that pointers (and space) returned from the transport remain valid until 1) the transport is released or 2) the push/pop call is made against the transport? That is in fact what I intended for push. For pop this would place a lot more restrictions on the engine implementation. Say for example peek is called and then the top half is used to write message data. Ideally there should actually be more data to write over the network, which means that the transport may want to grow (realloc) the output buffer, and this of course is more complex if the external pointer needs to stay valid. Given that at worst this will incur an extra copy that is equivalent to what we're currently doing, I figured it would be safer to start out with more conservative semantics. We can always relax them later when we have had more time to consider the implementation. Or perhaps a reference count-based interface would be safer? Once the driver determines there is capacity/pending, it reserves the buffer and is required to push/pop to release it? Oh, and those names absolutely stink. ;) It's unclear from the function names what components of the transport they are affecting. I'd rather something more readable: pn_transport_capacity() -- pn_transport_input_capacity() pn_transport_buffer() - pn_transport_input_buffer() pn_transport_push() -- pn_transport_input_written() I think your names (and my documentation) actually suffer from exposing too much of the implementation. There aren't necessarily distinct input/output buffer objects, rather the whole transport interface itself is really just single structure (a [transforming] byte queue) with push/peek/pop corresponding exactly to any standard queue interface. FWIW, I do agree pn_transport_buffer() could be better, however capacity, pending, push, peek, and pop all correspond to pretty much every standard queue/circular buffer/stack interface I've ever seen, and my hasty documentation notwithstanding, I think that is a good meme to take advantage
Re: transport interface changes
This seems to require more copying, right? I'm always weary of extra copying. Is there a way to have a (small) pool of buffers, or just two for swapping between, that can be used to eliminate extra copying? Does that make sense in this case? Perhaps I'm missing something but as I said it sounds like extra copying which causes higher latency. William - Original Message - What we've got here is failure to communicate. There aren't necessarily distinct input/output buffer objects, rather the whole transport interface itself is really just single structure (a [transforming] byte queue) with push/peek/pop corresponding exactly to any standard queue interface. Aha! Well, that explains it - I've always though that the transport was composed of two separate buffers - one for input, the other for output. At least, that's my interpretation of the existing API. A transforming byte queue didn't immediately pop into my mind when reading these new APIs. You may want to add a bit of documentation to that patch explaining this meme before the APIs are described. Would be quite useful to anyone attempting to implement a driver. -K - Original Message - Looks like the attachement didn't make it. Here's the link to the patch on JIRA: https://issues.apache.org/jira/secure/attachment/12568408/transport.patch --Rafael On Thu, Feb 7, 2013 at 8:10 AM, Rafael Schloming r...@alum.mit.edu wrote: Here's a patch to engine.h with updated docs/naming. I've documented what I believe would be future lifecycle semantics, however as I said before I think an initial patch would need to be more conservative. I think these names would also work with input/output prefixes, although as the interface now pretty much exactly matches a circular buffer (i.e. a byte queue), I find the input/output prefixes a bit jarring. --Rafael On Thu, Feb 7, 2013 at 5:53 AM, Rafael Schloming r...@alum.mit.edu wrote: On Wed, Feb 6, 2013 at 5:12 PM, Rafael Schloming r...@alum.mit.eduwrote: On Wed, Feb 6, 2013 at 2:13 PM, Ken Giusti kgiu...@redhat.com wrote: Rafi, I agree with the rational behind these changes. /** Return a pointer to a transport's input buffer. This pointer may * change when calls into the engine are made. I think we'll need to be a little more liberal with the lifecycle guarantee of these buffer pointers. Drivers based on completion models (rather than sockets) could be forced to do data copies rather than supplying the pointer directly to the completion mechanism. That sentence was actually supposed to be deleted. The sentences after that describes the intended lifecycle policy for the input buffer: Calls to ::pn_transport_push may change the value of this pointer and the amount of free space reported by ::pn_transport_capacity. Could we instead guarantee that pointers (and space) returned from the transport remain valid until 1) the transport is released or 2) the push/pop call is made against the transport? That is in fact what I intended for push. For pop this would place a lot more restrictions on the engine implementation. Say for example peek is called and then the top half is used to write message data. Ideally there should actually be more data to write over the network, which means that the transport may want to grow (realloc) the output buffer, and this of course is more complex if the external pointer needs to stay valid. Given that at worst this will incur an extra copy that is equivalent to what we're currently doing, I figured it would be safer to start out with more conservative semantics. We can always relax them later when we have had more time to consider the implementation. Or perhaps a reference count-based interface would be safer? Once the driver determines there is capacity/pending, it reserves the buffer and is required to push/pop to release it? Oh, and those names absolutely stink. ;) It's unclear from the function names what components of the transport they are affecting. I'd rather something more readable: pn_transport_capacity() -- pn_transport_input_capacity() pn_transport_buffer() - pn_transport_input_buffer() pn_transport_push() -- pn_transport_input_written() I think your names (and my documentation) actually suffer from exposing too much of the implementation. There aren't necessarily distinct input/output buffer objects, rather the whole transport interface itself is really just single structure (a [transforming] byte queue) with push/peek/pop corresponding exactly to any standard queue interface. FWIW, I do agree pn_transport_buffer() could be better, however
Re: transport interface changes
On Thu, Feb 7, 2013 at 11:13 AM, William Henry whe...@redhat.com wrote: This seems to require more copying, right? I'm always weary of extra copying. Is there a way to have a (small) pool of buffers, or just two for swapping between, that can be used to eliminate extra copying? Does that make sense in this case? Perhaps I'm missing something but as I said it sounds like extra copying which causes higher latency. It actually eliminates a copy on both input and output relative to what we do now because the driver can copy directly into/out of the transport-owned buffers. --Rafael
Re: transport interface changes
My 2 cents on the naming issue: I'm not convinced that a single queue is the best metaphor for the Transport, even if qualified by the term transforming. The meaning of the input and output data is surely so different that calling it a queue masks the essence of what the engine does. To me, a transforming queue suggests something that spits out something semantically identical to its input. For example, a byte queue whose head is a UTF-8-encoded transformation of its UTF-8 tail. I don't think Transport falls into this category, therefore my preference would be for the words input and output to appear in the function names. Phil On 7 February 2013 14:23, Ken Giusti kgiu...@redhat.com wrote: What we've got here is failure to communicate. There aren't necessarily distinct input/output buffer objects, rather the whole transport interface itself is really just single structure (a [transforming] byte queue) with push/peek/pop corresponding exactly to any standard queue interface. Aha! Well, that explains it - I've always though that the transport was composed of two separate buffers - one for input, the other for output. At least, that's my interpretation of the existing API. A transforming byte queue didn't immediately pop into my mind when reading these new APIs. You may want to add a bit of documentation to that patch explaining this meme before the APIs are described. Would be quite useful to anyone attempting to implement a driver. -K - Original Message - Looks like the attachement didn't make it. Here's the link to the patch on JIRA: https://issues.apache.org/jira/secure/attachment/12568408/transport.patch --Rafael On Thu, Feb 7, 2013 at 8:10 AM, Rafael Schloming r...@alum.mit.edu wrote: Here's a patch to engine.h with updated docs/naming. I've documented what I believe would be future lifecycle semantics, however as I said before I think an initial patch would need to be more conservative. I think these names would also work with input/output prefixes, although as the interface now pretty much exactly matches a circular buffer (i.e. a byte queue), I find the input/output prefixes a bit jarring. --Rafael On Thu, Feb 7, 2013 at 5:53 AM, Rafael Schloming r...@alum.mit.edu wrote: On Wed, Feb 6, 2013 at 5:12 PM, Rafael Schloming r...@alum.mit.eduwrote: On Wed, Feb 6, 2013 at 2:13 PM, Ken Giusti kgiu...@redhat.com wrote: Rafi, I agree with the rational behind these changes. /** Return a pointer to a transport's input buffer. This pointer may * change when calls into the engine are made. I think we'll need to be a little more liberal with the lifecycle guarantee of these buffer pointers. Drivers based on completion models (rather than sockets) could be forced to do data copies rather than supplying the pointer directly to the completion mechanism. That sentence was actually supposed to be deleted. The sentences after that describes the intended lifecycle policy for the input buffer: Calls to ::pn_transport_push may change the value of this pointer and the amount of free space reported by ::pn_transport_capacity. Could we instead guarantee that pointers (and space) returned from the transport remain valid until 1) the transport is released or 2) the push/pop call is made against the transport? That is in fact what I intended for push. For pop this would place a lot more restrictions on the engine implementation. Say for example peek is called and then the top half is used to write message data. Ideally there should actually be more data to write over the network, which means that the transport may want to grow (realloc) the output buffer, and this of course is more complex if the external pointer needs to stay valid. Given that at worst this will incur an extra copy that is equivalent to what we're currently doing, I figured it would be safer to start out with more conservative semantics. We can always relax them later when we have had more time to consider the implementation. Or perhaps a reference count-based interface would be safer? Once the driver determines there is capacity/pending, it reserves the buffer and is required to push/pop to release it? Oh, and those names absolutely stink. ;) It's unclear from the function names what components of the transport they are affecting. I'd rather something more readable: pn_transport_capacity() -- pn_transport_input_capacity() pn_transport_buffer() - pn_transport_input_buffer() pn_transport_push() -- pn_transport_input_written() I think your names (and my documentation) actually suffer from exposing too much of the implementation. There aren't necessarily distinct input/output
Re: transport interface changes
Rafi, I agree with the rational behind these changes. /** Return a pointer to a transport's input buffer. This pointer may * change when calls into the engine are made. I think we'll need to be a little more liberal with the lifecycle guarantee of these buffer pointers. Drivers based on completion models (rather than sockets) could be forced to do data copies rather than supplying the pointer directly to the completion mechanism. Could we instead guarantee that pointers (and space) returned from the transport remain valid until 1) the transport is released or 2) the push/pop call is made against the transport? Or perhaps a reference count-based interface would be safer? Once the driver determines there is capacity/pending, it reserves the buffer and is required to push/pop to release it? Oh, and those names absolutely stink. ;) It's unclear from the function names what components of the transport they are affecting. I'd rather something more readable: pn_transport_capacity() -- pn_transport_input_capacity() pn_transport_buffer() - pn_transport_input_buffer() pn_transport_push() -- pn_transport_input_written() etc. -K - Original Message - A recent bug (PROTON-222) has exposed an issue with the transport interface. The details of the bug are documented in the JIRA, but the basic issue is that given the current transport interface there is no way for an application to discover via the engine interface when data has been actually written over the wire vs just sitting there waiting to be written over the wire. Note that by written over the wire I mean copied from an in-process buffer that will vanish as soon as the process exits, to a kernel buffer that will continue to (re)-transmit the data for as long as the machine is alive, connected to the network, and the remote endpoint is still listening. To understand the issue, imagine implementing a driver for the current transport interface. It might allocate a buffer of 1K and then call pn_transport_output to fill that buffer. The transport might then put say 750 bytes of data into that buffer. Now imagine what happens if the driver can only write 500 of those bytes to the socket. This will leave the driver buffering 250 bytes. The engine of course has no way of knowing this and can only assume that all 750 bytes will get written out. A somewhat related issue is the buffering ownership/strategy between the transport and the driver. There are really three basic choices here: 1) the driver owns the buffer, and the transport does no buffering 2) the transport owns the buffer, and the driver does no buffering 3) they both own their own buffers and we copy from one to the other Now the division between these isn't always static, there are hybrid strategies and what not, however it's useful to think of these basic cases. The current transport interface (pn_transport_input/output) and initial implementation was designed around option (1), the idea behind the pn_transport_output signature was that the engine could directly encode into a driver-owned buffer. This, however, turned out to introduce some unfriendly coupling. Imagine what happens in our hypothetical scenario above if the driver has a 1K buffer and the engine negotiates a 4K frame size. The engine might end up getting stuck with a frame that is too large to encode directly into the driver's buffer. So to make the interface more friendly, we modified the implementation to do buffering internally if necessary, thus ending up in some ways closer to option (3). Now the reason this buffering issue is related to PROTON-222 is that one way to allow the engine to know whether data is buffered or not is to redefine the interface around option (2), thereby allowing the engine to always have visibility into what is/isn't on the wire. This would also in some cases eliminate some of the extra copying that occurs currently due to our evolution towards option (3). Such an interface would look something like this: // deprecated and internally implemented with capacity, buffer, and push ssize_t pn_transport_input(pn_transport_t *transport, const char *bytes, size_t available); // deprecated and internally implemented with pending, peek, and pop ssize_t pn_transport_output(pn_transport_t *transport, char *bytes, size_t size); /** Report the amount of free space in a transport's input buffer. If * the engine is in an error state or has reached the end of stream * state, a negative value will be returned indication the condition. * * @param[in] transport the transport * @return the free space in the transport */ ssize_t pn_transport_capacity(pn_transport_t *transport); /** Return a pointer to a transport's input buffer. This pointer may * change when calls into the engine
Re: transport interface changes
On Wed, Feb 6, 2013 at 2:13 PM, Ken Giusti kgiu...@redhat.com wrote: Rafi, I agree with the rational behind these changes. /** Return a pointer to a transport's input buffer. This pointer may * change when calls into the engine are made. I think we'll need to be a little more liberal with the lifecycle guarantee of these buffer pointers. Drivers based on completion models (rather than sockets) could be forced to do data copies rather than supplying the pointer directly to the completion mechanism. That sentence was actually supposed to be deleted. The sentences after that describes the intended lifecycle policy for the input buffer: Calls to ::pn_transport_push may change the value of this pointer and the amount of free space reported by ::pn_transport_capacity. Could we instead guarantee that pointers (and space) returned from the transport remain valid until 1) the transport is released or 2) the push/pop call is made against the transport? That is in fact what I intended for push. For pop this would place a lot more restrictions on the engine implementation. Say for example peek is called and then the top half is used to write message data. Ideally there should actually be more data to write over the network, which means that the transport may want to grow (realloc) the output buffer, and this of course is more complex if the external pointer needs to stay valid. Given that at worst this will incur an extra copy that is equivalent to what we're currently doing, I figured it would be safer to start out with more conservative semantics. We can always relax them later when we have had more time to consider the implementation. Or perhaps a reference count-based interface would be safer? Once the driver determines there is capacity/pending, it reserves the buffer and is required to push/pop to release it? Oh, and those names absolutely stink. ;) It's unclear from the function names what components of the transport they are affecting. I'd rather something more readable: pn_transport_capacity() -- pn_transport_input_capacity() pn_transport_buffer() - pn_transport_input_buffer() pn_transport_push() -- pn_transport_input_written() I think your names (and my documentation) actually suffer from exposing too much of the implementation. There aren't necessarily distinct input/output buffer objects, rather the whole transport interface itself is really just single structure (a [transforming] byte queue) with push/peek/pop corresponding exactly to any standard queue interface. FWIW, I do agree pn_transport_buffer() could be better, however capacity, pending, push, peek, and pop all correspond to pretty much every standard queue/circular buffer/stack interface I've ever seen, and my hasty documentation notwithstanding, I think that is a good meme to take advantage of, particularly since the push/peek/pop semantics and their relationship to each other is actually quite important in guaranteeing that the pitfall occurring in PROTON-222 doesn't come up when people code their own drivers. --Rafael
