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

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?

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

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

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

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

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

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

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

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

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

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

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

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

> 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!)
  * may stop using transport T (and export it)

Transport:

  * reads and writes byte streams
  * supports low-level protocol layering and/or switching

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.

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.

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

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

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

Thank you,

Alex.
Received on Sat Mar 01 2014 - 22:35:21 MST