Re: [PATCH] Comm::TcpReceiver - part 1, 2, and 3

On 2/03/2014 11:34 a.m., Alex Rousskov wrote:
> On 03/01/2014 02:55 AM, Amos Jeffries wrote:
>> On 1/03/2014 5:23 p.m., Alex Rousskov wrote:
>>> Agent: Common base for Client and Server agents. Uses Transport.
>
>>> Client: Common base for HttpClient, FtpClient, IcapClient, etc.
>>> Server: Common base for HttpServer, FtpServer, etc.
>>> HttpClient: Sends HTTP requests. Receives HTTP responses.
>>> FtpServer: Receives FTP commands. Sends FTP replies.
>>> ...
>
>>> TransportLayer: Common base for TcpLayer, SslLayer, and other layers.
>>> TcpLayer: A final TransportLayer using a TCP socket Comm APIs for I/O.
>>> SslLayer: A TransportLayer using another TransportLayer for I/O.
>>> ...
>
>>> Connection: A socket with a peer address. We have that already.
>
>
>> Problem: the Comm:: layer and fd_* layer APIs provide an abstracted sent
>> of functions that (currently) provide *all* of the above layers behind a
>> single open/close/read/write/set-timeout API. In similar separation as
>> OSI layer 4 and layer 5 have a definite shear line between them.
>>
>> fd_* provides the socket state API. While Comm:: / comm_* provides the
>> semantics abstraction API.
>
> Yes, but where is the problem?
>

Nevermind you cleared it up below.

>
>>> Transport: Contains one or more ConnLayers. Handles I/O multilayering.
>>
>> ConnLayer? are you meaning UDP/TCP/SSL FD and read(2)/write(2)/etc.
>
> Sorry, typo: Should have been TransportLayer as defined above. That is:
>
> Transport: Contains one or more TransportLayers.
> Handles I/O multi-layering or stacking.
>
>
>> Please can you also explain what you mean by "I/O multilayering". You
>> seem to be defining it as something separate from both
>> UDP/TCP/TLS/SSL/etc low-level protocols (xLayer above) and
>
> UDP/TCP and TLS/SSL can be layered/stacked on top of each other (and are
> already layered in some cases).
>
>
>> HTTP/FTP/etc high-level protocols (clients/servers above).
>
> HTTP/FTP/etc are not layered/stacked on top of each other in this model.
> It is not impossible to stack them, but the described model does not go
> far enough to reach that extreme. It offers a separation of "low-level"
> protocols (various Transport-related classes) and "high-level" protocols
> (various Agents). Hopefully, we can keep it that way.

Got it.

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?

 BTW either type can be multi-layered, but only within their level.

>
>> My understanding of the term is protocol stacks like
>> WebSockets-over-HTTP-over-TLS-over-TCP-over-IP-over-...
>>
>> I am also struggling to see how what it does differs from TransportLayer.
>
> TransportLayer is a single layer (e.g., TCP or SSL). Transport is a
> stack of those single layers. The common case is a stack containing a
> single transport layer (e.g., TCP). Current Comm hides that complexity
> by allowing only one set of "I/O" methods for any socket. We may be able
> to keep it that way, but that remains to be seen. I do not think the
> final outcome is important at this time.
>
>
>> Overall my impression of this description is that Comm:: and comm_*()
>> API is the Transport -> xLayer -> TransportLayer stack of classes
>> without any actual class wrapper.
>> Am I right?
>
> Yes, you are right, provided I interpret "without any actual class
> wrapper" correctly. All those transport things are pretty much meant to
> do what Comm does today.
>
> Please note that I am not advocating rewriting Comm and this point! We
> may never need the full implementation of the above hierarchy. However,
> some cheap wrappers may be needed and it is nice to have a picture of
> the full concept in front of us when working on any particular piece.
>

Phew ;-).

>
>> FWIW: Since these transport pieces all seem to be scoped at where the
>> Comm:: and comm_* and fd_* code APIs exist today I'm doubtful we should
>> be even discussing them much further at this stage. This project patch
>> is explicitly trying *not* to change any of that layers code just yet.
>
> Agreed. However, it is important to draw lines that separate TcpReceiver
> from Comm and from, say, ClientAgent. My design sketch attempts to draw
> such lines. Your sketch may be different, but the lines have to be
> drawn. It is important to know whether TcpReceiver is a transport class
> that lives inside Comm or is a protocol agent class working above the
> Comm layer.
>
>
>> I thought TcpReceiver would be what you define Transport. But I agree it
>> does seem to be far too high level semantics for that.
>
> Agreed. And while that class was suspended between those two levels (or
> trying to be in both of them at the same time), it was very difficult to
> both write and review it correctly. I think we are in the process of
> removing that roadblock now.
>
>
>>> Whether Agent is a dynamically assigned member of some other class
>>> should not be relevant to what Agent is. We should strive to define
>>> things for what they are/do, not for how they may be used (to the extent
>>> possible).
>>>
>>> IIRC what ConnStateData does today, it is an HttpServer in the above
>>> definition (i.e., an Agent).
>>
>> ConnStateData in trunk seems to be a conflated base Agent with HttpAgent
>> and HttpServer operations.
>
> Yes, the code that is supposed to be extracted/shared is not. And your
> observation below is even more important as far as mapping new
> HttpServer to old classes is concerned.
>
>
>> Ideally the HttpServer operations are more properly done by
>> ClientSocketContext via the use of ClientHttpRequest for the message
>> semantic operations.
>
> You are probably right, but let's view all those old ConnStateData and
> Client* classes as a single new HttpServer blob for now. The exact
> relationships inside that blob are pretty messy and need careful
> dissection/analysis. Let's cement our progress with Agent first.
>

Agreed.

>
>>> Overall, I think your TcpReceiver class is stuck somewhere between
>>>
>>> * Agent (that is how you actuall use TcpReceiver in the patch!) and
>>>
>>> * Transport (that is how you talk about TcpReceiver on this thread when
>>> explaining/justifying some of its internals).
>>>
>>
>> Agreed. However I think its only stuck-between in the details that its
>> been placed in Comm:: where the Agent semantics dont make sense, and
>> named wrongly after a Transport API which it does not specifically provide.
>
> I think it is a lot more than just that (or this thread would not have
> been so long), but that is not important right now. If we agree on the
> direction, then I am sure we can get there fast enough, regardless of
> the starting point.
>

Sure. This Agent direction is fine by me.

>
>> 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.

>
>>> This is just my impression, of course! If I am on the right track, then
>>> I suggest you pick the Agent design because
>>>
>>> a) that is how you _use_ your class, which is more important than the
>>> implementation details; and
>>>
>>> b) it is usually easier to design and implement classes you can use
>>> immediately rather than classes that you have to just keep around for
>>> future use.
>
>
>> Okay. I agree on the design. I have been confused about how several of
>> the definitions map to Squid code (existing or after re-write). I hope
>> my questions and comments above have clarified where that confusion is.
>
> Yes, I think we may be able to finally make progress now. The key is to
> keep transport-related (i.e., low level) things in Comm and focus on
> Agent-related (i.e., high level) things.
>
> If you agree with my definition of Agent, the rest is mostly "simple":
> If ClientAgent does X and ServerAgent does X, then X should be in the
> Agent class. We still need to agree on what ClientAgent and ServerAgent
> are. And I sense that there may be a disagreement regarding whether an
> Agent operates on bytes or messages. More on both issues below.
>

I view it 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.

 Processing of those messages after parsing and before packing is what
FooClient/FooServer does to the messages to/from FooAgent.

>
>> Okay this is what I know of the protocols requirements:
>>
>> * HTTP/1 protocol Agent requires semantics for sending bytes and
>> receiving bytes over a single Transport.
>
> In my design, there is always a "single Transport". That single
> Transport may internally support several transport layers. HTTP/1 does
> not really care about those layers, beyond a couple of
> initialization/switching points for SslBump. In other words:
>
> HttpAgent uses a Transport object member for I/O.
>
> Today, that "Transport object" is a socket descriptor and the
> corresponding fde structure, essentially. Today, the I/O is done via
> Comm. We can (and should) keep it that way, for now, I think.
>

For now lets say I agree. We can revisit if any future code proves
problematic.

>
>> * 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.

>
>> * Secure-HTTP Agent (RFC2660 which I would like to implement soonish)
>> requires an HTTP/1 Agent.
>>
>> * ICY protocol Agent requires an HTTP/1 Agent.
>
> No comment (for the lack of knowledge of those protocols). I am pretty
> sure that by the time we agree on HTTP, FTP, and Tunnels, there will not
> be huge disagreements regarding other protocols.
>

Sure. Pointing out in advance that we need to account for multilayering
at the HTTP layer too.

>
>> * Tunnel (blind relay) requires two Agents with independent Transport each.
>> One for each of client and server channels.
>
> I do not recommend trying to implement tunnels that way. Conceptually,
> what you describe is OK, but I suspect that the corresponding
> implementation would be difficult and expensive performance-wise.
> Moreover, it would bloat Agent with unnecessary code and/or bring a lot
> of unnecessary code into the Tunnel class(es).
>
> Instead, I would try to implement a Tunnel as a single job that uses two
> Transport objects. If we must make Tunnel an Agent child, we can, but I
> suspect we do not need or want that. Tunnels do not operate on messages
> like Agents do; Tunnels operate on bytes (and I think that distinction
> is very important for the Agent API!). Tunnels are both clients and
> servers, but Agents are either clients or servers. Agents need
> adaptation hooks but Tunnels do not. Etc., etc.

Hmm. Semantic idea of taking the bytes from one Transport to push out
another is high-level in the tunnel model. see below.

>
>> * 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.

==> 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.

==> the FTP Agent code on ctrl and data channels are significantly
different enough to make it worth creating two Agents.

 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. 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 ...

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.

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.

 ==> It does not matter to the TcpReceiver/Agent base class whether its
child class is parsing the bytes first or just pushing them out as
opaque blob to somewhere else.

NP: with the above designs both TunnelAgent and FtpDataAgent can be
BodyPipe Producer and Consumer. The FTP one produces-only and the tunnel
both produce and sink opaque BodyPipe bytes.
 The BodyPipe behaviour is going to be important for HTTP/1<->HTTP/2
mapping of CONNECT requests. Since delivering CONNECT over an HTTP/2
peer will require packaging up the tunnel opaque "body" bytes as DATA
frames for multiplexing to the peer, and likewise de-muxing DATA frames
from a Http2Agent into a TunnelAgent.
 It also helps ssl-bump as the code doing bump can register as BodyPipe
sink for a TunnelAgent and decrypt, or register as producer for
TunnelAgent and re-encrypt for delivery.

>
>> In general the clients/servers need to be able to pass around and share
>> either Agent or Transport, whichever layer has control over both the
>> input buffer data and socket FD.
>
> I am not sure I can fully agree with that statement. Sharing of Agents
> is probably a bad idea in general because they are jobs and it is
> wrong/difficult to share and pass around jobs. Passing around Transport
> and buffers will be necessary indeed, but it probably happens outside of
> Agent scope.

Okay. Understood. Transport + buffer then.

>
>> 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.

Can we agree that:
 - all messages are represented as a sequence of bytes sent/received
over the Transport API.
 - the format of those bytes is determined by FooAgent, Transport sees
them as opaque.

> * may stop using transport T (and export it)
>
> Transport:
>
> * reads and writes byte streams
> * supports low-level protocol layering and/or switching
>

Okay.

> 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.

>
> If you insist that TcpReceiver must work with byte streams, I would
> suggest that you call and define that class as Transport. It would have
> a completely different purpose/design (as discussed earlier). If you
> continue to agree with my class hierarchy, then Agents are focused on
> processing messages. Agents do need to read and write bytes for that, of
> course, but they do that using Transport.
>

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.

>
>>>>>> '''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?

>
>>>>> In other words (and simplifying a bit):
>>>>>
>>>>> /// raw TCP bytes (WRONG: could be SSL-decrypted TCP)
>>>>> MemBuf inBuf;
>>>>>
>>>>> /// opaque received bytes (OK but a little vague)
>>>>> MemBuf inBuf;
>>>>
>>>> Explicitly accurate for what is in there though. Which is why I left it
>>>> undocumented. "inBuf" seems clear enough.
>>>
>>> There is a significant difference among the three inBuf descriptions I
>>> have posted so, no, inBuf without a description is not clear enough.
>>>
>>
>> Fine. "opaque received bytes" it is then.
>
>
> In my sketch, Transport uses opaque bytes. Agents deal with high-level
> protocol message bytes.
>
> * Does the abstract Agent class know what high-level protocol those
> message bytes represent? No.
>
> * Does the abstract Agent class know where one message stops and the
> other one begins? No.
>
> However, the abstract Agent class should have a concept of an incoming
> message (something one extracts from the input buffer using parsing) and
> perhaps even a concept of a message stream (multiple messages stored
> sequentially in the input buffer). Why? Because all of its kids have
> those concepts.
>
>
> To summarize, I think there are two important design decisions that we
> still need to agree on:
>
> 1) Whether the abstract Agent class deals with opaque bytes or
> high-level protocol messages. In my sketch, it is messages because all
> Agent kids deal with messages and the abstract Agent class has to
> encapsulate concepts shared by all of its kids. This distinction becomes
> the line that separates low-level from high-level, Transport from Agent.
> Thus, it is critical.
>
> 2) Whether ServerAgent deals with just one message or all incoming
> messages from a single Transport. This is a difficult question that I
> prefer to discuss after (1) is resolved.
>

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/

>
>>>> +bool
>>>> +Comm::TcpReceiver::injectFakeRequestXXX(MemBuf &fake)
>>> ...
>>>>>>> Overall, I am not sure this method is needed. The caller can do the
>>>>>>> manipulations on its own, right? It may be easier for the caller to add
>>>>>>> the required capacity checks than implement this general code correctly.
>>>
>>>>>> Ideally not. I am trying to move the inBuf to a private member so the
>>>>>> childs should only be playing with the buffer they get passed by
>>>>>> processReadBuffer(MemBuf &)
>>>
>>>>> As you know by now, I do not think that hiding input buffer from kids is
>>>>> going to work unless you force kids to copy it. TcpReceiver does not
>>>>> control all the events in a transaction life and, hence, cannot prohibit
>>>>> access to the buffered message which may be waiting for those events.
>>>
>>>> By making this a private/protected member we restrict access to it from
>>>> objects outside the child class and restrict parsing/processing actions
>>>> on it to the code where it is passed as parameter to
>>>> processReadbuffer(Membuf &).
>>>
>>> Yes, but we were discussing whether injectFakeRequestXXX() is needed, to
>>> which you replied that you are trying to make inBuf private (not
>>> protected!). So, if you now agree that kids should have access to the
>>> buffer, let's remove injectFakeRequestXXX() with all if its problems.
>>>
>>
>> That was my reasons for making it private. Not agreement on keeping it
>> public. We want the I/O data to have to pass through parsing/processing
>> layer before anything else gets access to its contents.
>>
>>
>> Also, I do not as yet see any reason to avoid having it private in the
>> Agent model.
>
> IIRC, I gave you specific technical reasons why kids need buffer access
> that is not initiated by Agent: Agents that process external events such
> as Store/ICAP/eCAP notifications need access to the input buffer to
> handle those events. Since those events are not triggered by Agent's
> Transport, the code in the abstract Agent class would not be able to
> give those agents the buffer when they need it.
>
> To satisfy those needs, you can:
>
> * Remove inBuf from Agent. Let kids manage it.
> * Make Agent::inBuf accessible to kids.
>
> I recommend the latter because all concepts shared by all Agent kids
> should be supported by the Agent class itself.
>
>> If we do need it in future fine, but this effort has
>> revealed only a few places abusing the buffer that need to be fixed and
>> I'm not happy conflating this big patch with that extra work just yet.
>
> AFAICT, all is required now is making inBuf protected and removing code
> that you have added only because inBuf was private. This is not a lot of
> extra work, is it?
>
> This is not just hand waving about a minor issue. Whether kids have
> access to the input buffer affects Agent design (as the code you were
> forced to add illustrates).
>

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.
 a) because the message is either parsed immediately, or
 b) more bytes are needed and another call to processReadBuffer() will
happen later, or
 c) shunting the bytes down BodyPipe requires a Call loop in the kid
agent which needs to track the given MemBuf

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
which should correctly be in the HttpClient in your model not a member
of the HttpAgent child (ConnStateData).

Amos
Received on Sun Mar 02 2014 - 04:43:44 MST