You are viewing a plain text version of this content. The canonical link for it is here.
Posted to proton@qpid.apache.org by Rafael Schloming <rh...@alum.mit.edu> on 2013/02/06 18:14:46 UTC
transport interface changes
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 to
* ::pn_transport_push may change the value of this pointer and the
* amount of free space reported by ::pn_transport_capacity.
*
* @param[in] transport the transport
* @return a pointer to the transport's input buffer
*/
char *pn_transport_buffer(pn_transport_t *transport);
/** Push data from a transport's input buffer into the engine and
* return how much data was succesfully pushed.
*
* @param[in] transport the transport
* @param[size] the amount of data in the transport's input buffer
* @return the amount of data consumed
*/
size_t pn_transport_push(pn_transport_t *transport, size_t size);
/** Return the number of pending output bytes for a transport, or a
* negative error code if the engine is in an error state.
*
* @param[in] the transport
* @return the number of pending output bytes, or an error code
*/
ssize_t pn_transport_pending(pn_transport_t *transport);
/** Return a pointer to a transport's output buffer. Any calls into
* the engine may change the value of this pointer and it's contents.
*
* @param[in] the transport
* @return a pointer to the transport's output buffer
*/
const char *pn_transport_peek(pn_transport_t *transport);
/** Remove bytes from the head of the output buffer for a transport.
*
* @param[in] the transport
* @param[size] the number of bytes to remove
*/
void pn_transport_pop(pn_transport_t *transport, size_t size);
This new interface basically models the transport as a queue of bytes. On
the input side, pn_transport_input is decomposed into three separate parts:
capacity, buffer, and push. On the output side, pn_transport_output is
decomposed into three separate parts: pending, peek, and pop. These changes
would simplify the current driver implementation as it would no longer need
to do it's own buffering, and usage of the peek/pop style interface on the
output side would be a much safer interface for driver's in general and
allow the engine to know what has/hasn't made it onto the wire. To revisit
our hypothetical scenario, the driver no longer needs to bother with its 1K
buffer, it simply queries the transport for pending bytes, offers them all
to the socket write, and then pops only those bytes that have actually been
written.
This also moves the transport interface firmly into the camp of option (2).
It's somewhat unavoidable that the transport needs to do it's own
buffering, particularly when you consider things like SSL, and exposing
that fact at least allows the option of a driver that avoids the copy. It
should be noted though that there are some reasons that a driver may want
to own it's own buffers, and in these cases an extra copy will be
necessary, resulting again in something that looks like option (3). There
are ways we might address this in the future by providing hooks into the
allocation that the transport does, however I don't believe that affects
the part of the interface presented above.
Please have a look over this and let me know what your thoughts are. I'd
appreciate some comments from the Java folks on how to address this problem
for the Java transport interface. I don't think the buffer management needs
to be identical to the C as the whole buffer ownership problem is a bit
different with garbage collection (e.g. it's less of a problem to create
buffers in one component and pass ownership to another component), however
I believe the interface does need to address the same thing that the
peek/pop portion of this proposal is addressing, i.e. adding some means of
confirming when the bytes are actually "on the wire".
--Rafael
Re: transport interface changes
Posted by Rafael Schloming <rh...@alum.mit.edu>.
On Thu, Feb 7, 2013 at 8:20 AM, Phil Harvey <ph...@philharveyonline.com>wrote:
> 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 */
> }
>
Seems reasonable. Given my rusty recollection of ByteBuffer semantics,
would you mind spelling out what maps to pending and the size parameter to
pop? Also, what are the constraints in usage of the buffers? I understand
the use of ByteBuffer is needed in order to directly address C memory
without a copy, but am I right in thinking that not all usage of the
exposed ByteBuffer's would be safe, e.g. it would probably mess with the
transport to call flip or compact the byte array or something like that. Is
there an easy way to describe what is/isn't allowed?
--Rafael
Re: transport interface changes
Posted by Phil Harvey <ph...@philharveyonline.com>.
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 <rh...@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 to
> * ::pn_transport_push may change the value of this pointer and the
> * amount of free space reported by ::pn_transport_capacity.
> *
> * @param[in] transport the transport
> * @return a pointer to the transport's input buffer
> */
> char *pn_transport_buffer(pn_transport_t *transport);
>
> /** Push data from a transport's input buffer into the engine and
> * return how much data was succesfully pushed.
> *
> * @param[in] transport the transport
> * @param[size] the amount of data in the transport's input buffer
> * @return the amount of data consumed
> */
> size_t pn_transport_push(pn_transport_t *transport, size_t size);
>
>
> /** Return the number of pending output bytes for a transport, or a
> * negative error code if the engine is in an error state.
> *
> * @param[in] the transport
> * @return the number of pending output bytes, or an error code
> */
> ssize_t pn_transport_pending(pn_transport_t *transport);
>
> /** Return a pointer to a transport's output buffer. Any calls into
> * the engine may change the value of this pointer and it's contents.
> *
> * @param[in] the transport
> * @return a pointer to the transport's output buffer
> */
> const char *pn_transport_peek(pn_transport_t *transport);
>
> /** Remove bytes from the head of the output buffer for a transport.
> *
> * @param[in] the transport
> * @param[size] the number of bytes to remove
> */
> void pn_transport_pop(pn_transport_t *transport, size_t size);
>
> This new interface basically models the transport as a queue of bytes. On
> the input side, pn_transport_input is decomposed into three separate parts:
> capacity, buffer, and push. On the output side, pn_transport_output is
> decomposed into three separate parts: pending, peek, and pop. These changes
> would simplify the current driver implementation as it would no longer need
> to do it's own buffering, and usage of the peek/pop style interface on the
> output side would be a much safer interface for driver's in general and
> allow the engine to know what has/hasn't made it onto the wire. To revisit
> our hypothetical scenario, the driver no longer needs to bother with its 1K
> buffer, it simply queries the transport for pending bytes, offers them all
> to the socket write, and then pops only those bytes that have actually been
> written.
>
> This also moves the transport interface firmly into the camp of option (2).
> It's somewhat unavoidable that the transport needs to do it's own
> buffering, particularly when you consider things like SSL, and exposing
> that fact at least allows the option of a driver that avoids the copy. It
> should be noted though that there are some reasons that a driver may want
> to own it's own buffers, and in these cases an extra copy will be
> necessary, resulting again in something that looks like option (3). There
> are ways we might address this in the future by providing hooks into the
> allocation that the transport does, however I don't believe that affects
> the part of the interface presented above.
>
> Please have a look over this and let me know what your thoughts are. I'd
> appreciate some comments from the Java folks on how to address this problem
> for the Java transport interface. I don't think the buffer management needs
> to be identical to the C as the whole buffer ownership problem is a bit
> different with garbage collection (e.g. it's less of a problem to create
> buffers in one component and pass ownership to another component), however
> I believe the interface does need to address the same thing that the
> peek/pop portion of this proposal is addressing, i.e. adding some means of
> confirming when the bytes are actually "on the wire".
>
> --Rafael
>
Re: transport interface changes
Posted by Phil Harvey <ph...@philharveyonline.com>.
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 <ph...@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 <kg...@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 <rh...@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 <rh...@alum.mit.edu>
>> > > wrote:
>> > >
>> > >> On Wed, Feb 6, 2013 at 5:12 PM, Rafael Schloming
>> > >> <rh...@alum.mit.edu>wrote:
>> > >>
>> > >>> On Wed, Feb 6, 2013 at 2:13 PM, Ken Giusti <kg...@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
Posted by Rafael Schloming <rh...@alum.mit.edu>.
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 <ph...@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 <kg...@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 <rh...@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 <rh...@alum.mit.edu>
> > > > wrote:
> > > >
> > > >> On Wed, Feb 6, 2013 at 5:12 PM, Rafael Schloming
> > > >> <rh...@alum.mit.edu>wrote:
> > > >>
> > > >>> On Wed, Feb 6, 2013 at 2:13 PM, Ken Giusti <kg...@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
Posted by Phil Harvey <ph...@philharveyonline.com>.
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 <kg...@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 <rh...@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 <rh...@alum.mit.edu>
> > > wrote:
> > >
> > >> On Wed, Feb 6, 2013 at 5:12 PM, Rafael Schloming
> > >> <rh...@alum.mit.edu>wrote:
> > >>
> > >>> On Wed, Feb 6, 2013 at 2:13 PM, Ken Giusti <kg...@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
Posted by Rafael Schloming <rh...@alum.mit.edu>.
On Thu, Feb 7, 2013 at 11:13 AM, William Henry <wh...@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
Posted by Ken Giusti <kg...@redhat.com>.
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 <rh...@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
> > > <rh...@alum.mit.edu>
> > > wrote:
> > >
> > >> On Wed, Feb 6, 2013 at 5:12 PM, Rafael Schloming
> > >> <rh...@alum.mit.edu>wrote:
> > >>
> > >>> On Wed, Feb 6, 2013 at 2:13 PM, Ken Giusti <kg...@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
Posted by William Henry <wh...@redhat.com>.
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 <rh...@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
> > > <rh...@alum.mit.edu>
> > > wrote:
> > >
> > >> On Wed, Feb 6, 2013 at 5:12 PM, Rafael Schloming
> > >> <rh...@alum.mit.edu>wrote:
> > >>
> > >>> On Wed, Feb 6, 2013 at 2:13 PM, Ken Giusti <kg...@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
Posted by Ken Giusti <kg...@redhat.com>.
"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 <rh...@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 <rh...@alum.mit.edu>
> > wrote:
> >
> >> On Wed, Feb 6, 2013 at 5:12 PM, Rafael Schloming
> >> <rh...@alum.mit.edu>wrote:
> >>
> >>> On Wed, Feb 6, 2013 at 2:13 PM, Ken Giusti <kg...@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
Posted by Rafael Schloming <rh...@alum.mit.edu>.
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 <rh...@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 <rh...@alum.mit.edu> wrote:
>
>> On Wed, Feb 6, 2013 at 5:12 PM, Rafael Schloming <rh...@alum.mit.edu>wrote:
>>
>>> On Wed, Feb 6, 2013 at 2:13 PM, Ken Giusti <kg...@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
Posted by Rafael Schloming <rh...@alum.mit.edu>.
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 <rh...@alum.mit.edu> wrote:
> On Wed, Feb 6, 2013 at 5:12 PM, Rafael Schloming <rh...@alum.mit.edu> wrote:
>
>> On Wed, Feb 6, 2013 at 2:13 PM, Ken Giusti <kg...@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
Posted by Rafael Schloming <rh...@alum.mit.edu>.
On Wed, Feb 6, 2013 at 5:12 PM, Rafael Schloming <rh...@alum.mit.edu> wrote:
> On Wed, Feb 6, 2013 at 2:13 PM, Ken Giusti <kg...@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
Posted by Rafael Schloming <rh...@alum.mit.edu>.
On Wed, Feb 6, 2013 at 2:13 PM, Ken Giusti <kg...@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
Re: transport interface changes
Posted by Rafael Schloming <rh...@alum.mit.edu>.
On Wed, Feb 6, 2013 at 2:13 PM, Ken Giusti <kg...@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
Posted by Ken Giusti <kg...@redhat.com>.
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 are made. The amount of
> space in
> * this buffer is reported by ::pn_transport_capacity. Calls to
> * ::pn_transport_push may change the value of this pointer and
> the
> * amount of free space reported by ::pn_transport_capacity.
> *
> * @param[in] transport the transport
> * @return a pointer to the transport's input buffer
> */
> char *pn_transport_buffer(pn_transport_t *transport);
>
> /** Push data from a transport's input buffer into the engine and
> * return how much data was succesfully pushed.
> *
> * @param[in] transport the transport
> * @param[size] the amount of data in the transport's input
> buffer
> * @return the amount of data consumed
> */
> size_t pn_transport_push(pn_transport_t *transport, size_t size);
>
>
> /** Return the number of pending output bytes for a transport, or
> a
> * negative error code if the engine is in an error state.
> *
> * @param[in] the transport
> * @return the number of pending output bytes, or an error code
> */
> ssize_t pn_transport_pending(pn_transport_t *transport);
>
> /** Return a pointer to a transport's output buffer. Any calls
> into
> * the engine may change the value of this pointer and it's
> contents.
> *
> * @param[in] the transport
> * @return a pointer to the transport's output buffer
> */
> const char *pn_transport_peek(pn_transport_t *transport);
>
> /** Remove bytes from the head of the output buffer for a
> transport.
> *
> * @param[in] the transport
> * @param[size] the number of bytes to remove
> */
> void pn_transport_pop(pn_transport_t *transport, size_t size);
>
> This new interface basically models the transport as a queue of
> bytes. On
> the input side, pn_transport_input is decomposed into three separate
> parts:
> capacity, buffer, and push. On the output side, pn_transport_output
> is
> decomposed into three separate parts: pending, peek, and pop. These
> changes
> would simplify the current driver implementation as it would no
> longer need
> to do it's own buffering, and usage of the peek/pop style interface
> on the
> output side would be a much safer interface for driver's in general
> and
> allow the engine to know what has/hasn't made it onto the wire. To
> revisit
> our hypothetical scenario, the driver no longer needs to bother with
> its 1K
> buffer, it simply queries the transport for pending bytes, offers
> them all
> to the socket write, and then pops only those bytes that have
> actually been
> written.
>
> This also moves the transport interface firmly into the camp of
> option (2).
> It's somewhat unavoidable that the transport needs to do it's own
> buffering, particularly when you consider things like SSL, and
> exposing
> that fact at least allows the option of a driver that avoids the
> copy. It
> should be noted though that there are some reasons that a driver may
> want
> to own it's own buffers, and in these cases an extra copy will be
> necessary, resulting again in something that looks like option (3).
> There
> are ways we might address this in the future by providing hooks into
> the
> allocation that the transport does, however I don't believe that
> affects
> the part of the interface presented above.
>
> Please have a look over this and let me know what your thoughts are.
> I'd
> appreciate some comments from the Java folks on how to address this
> problem
> for the Java transport interface. I don't think the buffer management
> needs
> to be identical to the C as the whole buffer ownership problem is a
> bit
> different with garbage collection (e.g. it's less of a problem to
> create
> buffers in one component and pass ownership to another component),
> however
> I believe the interface does need to address the same thing that the
> peek/pop portion of this proposal is addressing, i.e. adding some
> means of
> confirming when the bytes are actually "on the wire".
>
> --Rafael
>