I suggest to pick one of the following options as the next step:
(A) Make TcpReceiver into a base Agent class as defined below. If this
approach results in significant code duplication, encapsulate duplicated
code into a yet unnamed class(es), such as a class discussed in (B).
This encapsulation is not expected to have drastic effects on the base
Agent interface itself or the Agent hierarchy interfaces in general.
(B) Make TcpReceiver into a yet unnamed class that is primarily
responsible for reading transport bytes and converting them into
transfer protocol message, messages, or their parts (e.g., body bytes in
FTP). Transfer protocol Agents will use objects of that new class to get
transfer messages or message parts. For example, an FtpClient agent will
use two objects of this class: One for the control connection and one
for the data connection.
In either case (either A or B), agree that an Agent object (of any leaf
Agent type) is responsible for coordinating all primary activities
related to handling of a given transfer protocol transaction (or
transactions, to be decided later) on a given Squid side. For example,
FtpClient agent is responsible for conducting an FTP transaction with an
FTP server (including both control and data channels).
Such an Agent object will use its own class code/members and the
code/members of its parent classes to accomplish that, as usual. The
base Agent class contains code/members needed by all Agents, of course.
This "obvious" paragraph was added just to make sure that you do not
think that I am proposing that the entire FTP handling code resides in a
single class. More general code that is usable/needed in other leaf
Agent classes will trickle up to parent classes, of course.
I recommend starting with (A). Justification is in the detailed
discussion below.
On 03/01/2014 09:43 PM, Amos Jeffries wrote:
> The IETF official line seems to be naming of these levels as Transport
> (UDP/TCP/etc) vs Transfer (HTTP/FTP/etc) protocols. Maybe we should
> mirror that terminology?
Sure, at least in discussions (and new code names where it makes sense).
Where does IETF place SSL/TLS in that division?
> Sure. This Agent direction is fine by me.
I think we still disagree on what Agent is, unfortunately (and, hence,
what your TcpReceiver is). I think you are currently not writing an
Agent class from my sketch. You are writing a class that does not
currently exist in my sketch. Details below.
>>> If I am understanding you right this class should be named AnyP::Agent
>>> in the generic protocol library instead of attempting to be part of
>>> Comm:: related to TCP.
>>
>> Yes, if that is what you want to implement (and, as you know, I think
>> that is a better choice than Transport). This may not be as easy as it
>> may seem. For example, "generic high-level protocol library" does not
>> work with "opaque bytes" it works with "protocol messages", but I am not
>> sure you are happy to accept that.
> If this TcpReceiver class is actually Agent then the child FooAgent's
> will be the ones receiving and emitting the messages. This being the
> base class that deals with the buffers send/receive operations as opaque
> bytes.
Yes. One of the problems in this discussion is that you resist the
notion of Agent dealing with protocol messages because it actually reads
opaque bytes. When thinking about a class hierarchy design, especially
the design of a base class, we should focus on the API it provides to
the kids and the rest of the world. From that point of view, you should
not resist saying "Agent deals with protocol messages" even though Agent
does not know what those messages are and even though Agent
implementation contains a read call that gets opaque bytes into the buffer.
In other words, when talking about a base Agent class, we should be
defining what an Agent (i.e., any object of any specific Agent class)
is, in terms general enough so that they are still applicable to every
Agent class. When defining Agent, we should not be focusing on what
specific code the base class has. That last part is relatively easy --
the base class will have code shared/needed by all of its kids!
If you wish, just think of those opaque bytes in buffer as transfer
protocol messages. Imagine that they become transfer protocol messages
when they leave the transport read call and enter Agent. They are still
in the same I/O buffer, but they are transfer protocol messages now.
Problem solved?
> I view [Agent] as the class taking messages in the transfer protocol and
> converting it to bytes out socket. Likewise taking opaque bytes from the
> socket and converting it to transfer protocol messages.
> ie the parser and possibly packer routines.
If you keep focusing on this packing/parsing bit, you will end up with a
completely different design where TcpReceiver is not an Agent but a
Reader+Parser (converting opaque bytes into protocol messages). That
[more complex] design is also valid and can be implemented, but we need
to be clear what we are implementing first because the implementations
of different designs will differ substantially.
In "TcpReceiver is a Parser" design:
Transport: Deals with transport layers (same as in my design).
Parser: Reads bytes from Transport and creates transfer messages.
Agent: An end point of a IETF transfer protocol using parser(s).
...
FtpClient: Coordinates work of FtpCtrlParser and FtpDataParser.
Also sends FTP commands and FTP data.
In mine design:
Transport: Deals with transport layers (same as in Parser design).
Agent: An end point of a IETF transfer protocol using transport(s).
...
FtpClient: Sends and receives FTP commands and FTP data.
Uses one Transport for receiving FTP commands and
another Transport for receiving FTP data.
Please note that I do not like "Parser" and "Packer" names for these new
classes because they initiate transport I/O in addition to packing and
parsing. At this time, I cannot offer a better name so I am using
Parser/Packer. We will need a better name (TransportEnd??) if we decide
to go forward with this design (which I hope we do not at this time).
The name "Agent" does not fit for this Reader+Parser class because an
agent is usually responsible for all (or most) aspects of some
transaction, not just unidirectional communication with another party.
For example, a good real estate agent does not just show you houses that
happen to be in the neighborhood, but also listens to your preferences
of what kind of house you are looking for, does market research, and
facilitates negotiations with the seller. Such an agent is responsible
for most interactions between you and the real estate "market". In my
sketch, Agent is the same kind of class, responsible for most transfer
protocol interactions with the outside world (on one side of Squid, and
in the scope of one transfer protocol connection or even one transfer
protocol message).
> Processing of those messages after parsing and before packing is what
> FooClient/FooServer does to the messages to/from FooAgent.
It is wrong to think of kid classes as receiving/sending something to
their parent classes. Sending/receiving is not a child-parent
relationship. It is a "pipe" or "message sending" relationship. I
suspect this difference is in the core of the current disagreements (or
more precisely "confusion") on this thread!
If you want FooClient to process messages to/from X, then X should not
be a parent of FooClient. X should be a class with no inheritance
relationship to FooClient. In this design, X is a Parser (and there can
be the corresponding Packer class, of course).
We do not have those Parser/Packer classes today (not to be confused
with the current/old packing API!), but we can create them and there are
certain advantages and disadvantages in doing so. In my design, these
classes do not exist (their work is done by Agents, without introducing
encapsulation boundaries).
We do not really need these classes yet, but we may need them in the
future, and we can create them now. Your TcpReceiver class would become
a Parser class (again, a better class name is needed!). One of the
biggest changes would be removal of TcpReceiver as a parent class of
existing Agent-like classes, making TcpReceiver objects their data
member(s), not parents.
I recommend avoiding this level of complexity for now. Once we have
functional Agent hierarchy (which is needed anyway!), and need to reuse
a transport-to-http converter in an Agent that deals with another
transfer protocol, we should be able to extract that code at that time.
>>> * HTTP/2 Agent requires semantics for sending bytes and receiving bytes
>>> over a single Transport which may also need to switch arbitrarily
>>> between TcpTransport and TlsTransport at byte boundaries as defined by
>>> the HTTP/2 protocol framing.
>>> NP: the peek-n-splice BIO methods over a TcpTransport would seem
>>> necessary for this.
>>
>> I know very little about HTTP/2, but I am willing to bet that the same
>> "single Transport" idea and the above sketch apply to HTTP/2 as well.
>>
>
> Think about ssl-bump applied to a CONNECT stream DATA frames which are
> multiplexed alongside several plaintext HTTP streams.
> HTTP/2 brings switching between TLS and non-TLS Transport at arbitrary
> byte boundaries.
FYI: To properly support the above using any of the designs discussed so
far, one would have to essentially elevate SSL to the transfer protocol
status. You cannot keep SSL in Comm/transport layer if you want to tell
OpenSSL which specific blocks of the input stream it needs to decode or
encode. We would have to start using OpenSSL as a buffer
encoding/decoding library instead of the socket encoding/decoding
library (i.e., a Transfer layer).
This is not a show stopper at all, of course. I do not think it affects
the current discussion as long as we agree that SSL/TLS can continue to
live in Comm/Transport layers for now. We will come back to this later
and add SSL/TLS filter objects where they are needed, removing SSL code
from Comm.
>>> * FTP requires two Agents requires semantics for sending bytes and
>>> receiving bytes over TcpTransport. Also the ability for both Agents to
>>> share a TcpTransport. Also data channel Agent requires ability to block
>>> ctrl channel Agent from reading.
>>> One for send/receive on ctrl channel, one for receive on data channel.
>>> Shared Transport when "data channel" is an ASCII mode transfer via ctrl
>>> socket.
>>
>> I think we just need one FtpAgent on each side of Squid: FtpClient and
>> FtpServer. The data channel is not a message channel (for the lack of a
>> better word) so it does _not_ require a separate Agent in my definition
>> of Agent. Here again, the distinction between low-level bytes (such as
>> FTP data bytes) and messages (such as FTP commands) is critical.
>>
>> This is similar to how an HttpServer agent may do DNS lookups (over a
>> UDP socket) or ICAP transactions (over a TCP connection). Those
>> auxiliary activities do not require a special HttpServer agent! They are
>> just a part of what an HttpServer does beyond its primary function:
>> processing of HTTP messages. Same for FTP.
>
> This is where your idea of Agent is separating from what I have planned.
Yes, this is where our plans differ substantially.
> ==> separating the opaque bytes read(2) and write(2) operations from FTP
> data channel and tunnelled connections will cause an unnecessary
> duplication of code which is already implemented by the base
> TcpReceiver/Agent class.
If we see code duplication, we can always move that code into a common
parent or a stand-alone object. Current TcpReceiver is not suitable for
reading data bytes anyway; you would have to dumb it down further if you
want to use it that way (and probably add a smarter class on top of it
for transfer protocols). For example, the object dedicated to reading
FTP data bytes from a transport connection should not have
sending-related functionality...
Also, if you keep TcpReceiver as an AsyncJob, you will need to go
through a significant redesign for FTP code that will need to manage at
least two async jobs that should not access each other directly. Not
impossible, and even brings certain benefits, but does require serious
work. I think it is best to avoid those complications until we actually
have to use that more complex design.
> ==> the FTP Agent code on ctrl and data channels are significantly
> different enough to make it worth creating two Agents.
Most of the complex FTP code is about _both_ ctrl and data channels so
keeping it in the same class may be a good idea. I agree that it is
_possible_ to separate that code into two classes, but see above
regarding the extra/complex work that would bring.
> Yes I can see a way that the FtpAgent could switch between ctrl and
> data sockets in a single Agent, BUT to do that would require pushing FTP
> protocol processing semantics into the Agent instead of the
> FtpClient/Server class.
Why would it leak FTP into Agent? I do not think such a leak is
required, and I would certainly be against it!
> Better IMO to have the Client/Server class use
> two Agents with different coded methods. One for parsing/packing the
> ctrl channel messages and the other shunting opaque data bytes into a
> BodyPipe ...
Most of the FTP code is not about parsing/packing/shunting. An FtpAgent
class focus is FTP protocol management. Coordination of control and data
exchanges as well as integration with Store, adaptation and other Squid
APIs consume a lot more code and effort than relatively primitive
actions such as parsing/packing/shunting. Again, if you want to isolate
reading-raw-bytes-and-parsing/packing/shunting parts of FTP into
separate classes, you can do that, but those separate classes would not
be Agent classes.
> The semantics of the FTP data channel are that it takes opaque bytes
> from a Transport and pushes them into a BodyPipe.
>
> The semantics of the tunnel connections are that it takes opaque bytes
> from a Transport and pushes them into another Transport.
>
> The semantics of the other protocol Client/Server connections (including
> FTP ctrl) are that they take opaque bytes from a Transport and parse
> into messages for handling.
Yes. Those are minor details as far as the Agent hierarchy is concerned.
FtpAgent focus is on FTP protocol complexities, not
reading-and-then-parsing/packing. Those details can be isolated, but at
a cost. I suggest that we do not isolate them until we see code
duplication that warrants such isolation.
> The basic concept of the Tcpreceiver/Agent design is that it takes
> opaque bytes and delivers it to a child class for handling the semantics.
As discussed above, it is wrong to use parent-child relationship for
message delivery. I am glad we reached that clarity level though. I
think we may be able to decide what to do with TcpReceiver at this
level: merge it into an Agent class (option A at the beginning of this
email) or convert it into a Parser class outside the Agent hierarchy
(option B).
>>> In summary overall Agent requires:
>>> * semantics for sending bytes
>>> * semantics for receiving bytes
>>> * ability to switch Transport type arbitrarily
>>> * ability to block another Agent reading on its Transport
>>> * maybe ability to switch between client/server owners/users.
>>
>> I would not put it that way. Here is how I would describe this:
>>
>> Agent:
>>
>> * owns/fully controls transport T at any given time
>> * uses transport T for sending and receiving messages (not bytes!)
>
> That "not bytes!" says that you want the message parsing/packing
> translation from bytes to HTTP/FTP messages to be done by Transport (
> ie. comm_read() ) and delivered _as messages_ to Agent.
Not, exactly: The very same byte in the input buffer is both a "raw
opaque byte" for Transport and "transfer message byte" for Agent. Agents
work with transfer protocol messages while Transport works with raw
bytes (or "transport protocol bytes" to be more precise).
> Can we agree that:
> - all messages are represented as a sequence of bytes sent/received
> over the Transport API.
Yes.
> - the format of those bytes is determined by FooAgent, Transport sees
> them as opaque.
Sure.
>> It is critical to draw the bytes/messages boundary like that IMO. Also,
>> it would be best to minimize Agent-to-Agent communication to the extent
>> possible. Most of that communication would go through other Squid layers
>> such as Store and BodyPipe.
>
> +1.
Note that having both FtpCtrlAgent and FtpDataAgent contradicts the
above because those two agents imply extensive Agent-to-Agent
communication and coordination: Both agents serve the same FTP
transaction! This is why those FtpCtrl* and FtpData* classes are not
really agents, they are Readers+Parsers. There may still be an FtpAgent
that coordinates their work, either as an explicit class (third job for
the same transaction end!) or an implicit concept spread around those
two Parser jobs (inferior design).
> TcpReceiver:
> * owns/fully controls transport T at any given time
> - Comm::Connection tcp; MemBuf inBuf;
>
> * uses Transport
> - Comm::Write + comm_*
>
> * may stop using transport T (and export it)
> - stopReceiving()/stopSending()/stopReadingXXX() API
> - exporting is TBD.
The above class is not a transfer protocol Agent IMO. It is just a
combination of Transport reading code and a transfer protocol parser.
Also, you should remove Comm::Write from the above. In your design, the
processing at TcpReceiver level is unidirectional. FtpDataReceiver does
not write FTP data.
>>>>>>> '''Common base for classes reading (and writing) from (and to) a single
>>>>>>> TCP Comm Connection. Contains basic connection management code and
>>>>>>> complex algorithms for coordinating I/O activity with message receiving
>>>>>>> (and sending) states.'''
>>>>>>
>>>>>> Please show me the TCP specific code in your TcpReceiver implementation.
>>>>>>
>>>>>
>>>>> The algorithm model behind readSomeData() assumes a streaming protocol
>>>>> with single input stream with serialized read(2).
>>>>> - eliminating direct use by SCTP, FTP, or other multi-connection protocols.
>>>>
>>>> I do not know about SCTP, but current TcpReceiver does not eliminate
>>>> direct use by FTP. FTP agents can use this code for the control channel.
>>>
>>> Ah. You are talking about FTP Agents controlling both channels. I am
>>> talking about a TcpReceiver being used on each channel separately.
>>
>> Yes, an Agent in my hierarchy sketch cannot control just a data channel.
>> Such an Agent would have no high-level protocol (i.e., FTP) messages to
>> send or receive, which is the primary purpose of an Agent in my sketch.
>>
>
> Ah. I see where we are thinking separately now.
>
> My model has a special-case Agent child class used by Tunnel and/or FTP
> data channel that receives an opaque byte stream and drops each
> Transport read(2) byte sequence into a BodyPipe as "a message" in Agent
> terms.
>
> That is what I mean by the second Agent for FTP. Are you okay with that?
No, I cannot accept such a special-case Agent child class. I can accept
it as a Reader+Parser class (called BodyReader in this case?), but it
would not be an Agent. Some agents might use such a BodyReader class as
a member or even one of the parents. This will be needed if we see
significant duplication of the corresponding code among Agents. I am not
against such encapsulation, under appropriate name, if/when it happens
to be needed. It is not needed today.
Please note that I am not sure that Tunnels should read raw bytes into
BodyPipes because those pipes would have to terminate at the same Tunnel
Agent, which feels like too much overhead and may not even be supported
by the current BodyPipe code. However, we can review that idea more
closely when we start working on Tunnels.
> Sounds like you want it documented as:
>
> /// Buffer for receiving messages.
> /// Filled by comm_read() and consumed by processReadBuffer().
>
> When Transport exists in future s/comm_read()/Transport/
I would drop the second line because it is not important and is not
accurate (e.g., some code injects bytes without comm_read usage and some
consumption will happen outside of processReadBuffer).
I would be a little more precise as far as the first line is concerned.
Something along these lines, perhaps?
/// received [transfer] protocol message bytes
/// kids parse this buffer to extract those messages
inBuf;
> Sure. Making protected.
>
> 1) I predict that you will find passing the buffer to
> processReadBuffer(MemBuf&) and the child retaining its own reference is
> all that is necessary for the kid classes.
Asking kids to retain a reference to a data member of a parent class is
a rather strange design: It would indeed allow us to keep inBuf private,
but essentially without any protections that such a privacy designation
offers _and_ making it harder to track buffer [mis]use (because there
are now more objects to track: the buffer object itself in the parent
and buffer pointers in kids).
> 2) Note that the code I was forced to add is not removable with change
> to protected. The injection hack is being done by a global function
This is fine. Just add a TODO comment to move the XXX method into the
right child class when possible.
> which should correctly be in the HttpClient in your model not a member
> of the HttpAgent child (ConnStateData).
HttpClient agent will have access to protected Agent class members,
including inBuf so that is not a problem AFAICT. Perhaps I misunderstood
what the above comment implies though.
Thank you,
Alex.
Received on Sun Mar 02 2014 - 21:27:48 MST
This archive was generated by hypermail 2.2.0 : Mon Mar 03 2014 - 12:00:12 MST