Re: [PATCH] client-side redesign pt1 - Comm::TcpReceiver

On 2014-01-22 18:45, Alex Rousskov wrote:
> On 01/07/2014 02:52 AM, Amos Jeffries wrote:
>> Updated patch attaced for audit.
>>
>> This one includes all the currently known bits for server-side delay
>> pools so no audit omissions this time around.
>>
>> On 4/01/2014 8:16 a.m., Alex Rousskov wrote:
>>> On 12/03/2013 10:05 PM, Amos Jeffries wrote:
>>>> This patch abstracts the TCP socket operations out of existing
>>>> client-side code into a class which can be re-used for any TCP
>>>> socket code.
>>>
>>>
>>>> +namespace Comm {
>>>> +
>>>> +class TcpReceiver : virtual public AsyncJob
>>>> +{
>>>> +public:
>>>
>>> Missing class description.
>
> You may want to add an "XXX: finalize and describe scope" comment in
> lieu of that description for now.
>
>
>> I was hoping initially to call it TcpReader, but these send-related
>> details prevent it being a pure read-only semantic. At least "receive"
>> semantics involve the concept of sending in the form of ACKs.
>
> To make matters worse, ICAP code is using read/write terms to talk
> about
> data being sent from/to Squid main code and receive/send terms to talk
> about data being sent from/to the remote ICAP service. See the first
> comment in src/adaptation/icap/ModXact.cc
>
> Unfortunately, I am worried there is a bigger problem here than the
> verb
> to use (on the other hand, our inability to name something is often
> indicative of a design problem; more on that immediately below).
>
>
>> Any better ideas at a name? The primary purpose is to control the
>> read(2) operations. The only write(2) related code is the stop*()
>> errors
>> flag state to indicate that sending has stopped or not. Which is
>> needed
>> to manage the close(2) properly. Or should we abstract that out to yet
>> another class managing FD lifetime?
>
> Difficult to say without doing the legwork. The correct design depends
> on what our clients and servers need. I am seriously worried that you
> are too focused on one server now (client_side_*cc) and once you start
> adding more and more agents, the current design would fall apart, even
> if we spend another five iterations perfecting it.
>
> All the TCP clients and servers you are willing to include (as future
> TcpReceiver kids) in the current project scope have at least one thing
> in common -- they all read and write protocol messages over
> [persistent]
> TCP connections. Why do you think it is a good idea to focus on just
> the
> reading part so much? If you want a common parent for all those agents,
> why not handle both actions in that common parent, especially if you
> already find it necessary to add a little bit of "send" part into the
> TcpReceiver?

The send part was supposedly done with Comm::Write API creation. Oh
well.

>
> If tomorrow we have both TcpReceiver and TcpSender, and all their kids
> have both classes as parents, with complex inter-parent dependencies
> that are already showing in TcpReceiver, we are doing something wrong.

Agreed.

>
> This may be one of those cases where restricting class scope too much
> makes the code more complex and less reusable. After reading your
> response to my initial comments, this is my primary concern for the
> overall design. I am not [just] ranting about it. This may be a serious
> matter.
>
> I can think of two very different ways to go forward from here:
>
> A) Forget about other agents, sides, etc. and focus on the HTTP server
> (i.e., client-side*cc) code exclusively. That code does not need a
> TcpReceiver. It needs a lot of work, but extracting TCP reading code
> into an abstract class is not one of them!
>
> B) Try to implement a common parent for all existing TCP protocol
> agents/sides. That common parent, let's call it TcpAgent for the
> purpose
> of this discussion, will have code to read and write messages. There is
> lots of common, complex code to extract from the existing agents into
> this common parent! If we go a step further, many Agents also maintain
> a
> connection pool, so you may add a TcpAgentWithPool class of some kind.
>
> IMO, both approaches above are valid in isolation, but mixing them may
> cause trouble.

(B) sounds like a good idea. Updating this class to present wrappers for
the Comm::Write API would be relatively simple compared to continuing
this design as read(2)-specific. Particularly given those
interdependencies that are popping up.

As for the pools. The high-level design I am aiming for has a TcpAgent
managing just one TCP connection. Pooling is at the client/server level
where the pool would be a set of TcpAgent child classes.

Amos
Received on Thu Jan 23 2014 - 01:17:01 MST