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.