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

On 01/22/2014 06:16 PM, Amos Jeffries wrote:
> On 2014-01-22 18:45, Alex Rousskov wrote:
>> 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.

AFAICT, Comm::Write API is just a pair of low-level "schedule write" and
"cancel write" calls. It is just the final step of writing a protocol
message into a socket. It is really not much different from a blocking
write(2) call with rate limiting underneath. Comm::Write works on a
completely different level compared to the code you are writing now.

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

Both options are good and both avoid read-specific design pitfalls. The
primary difference is that (A) ignores clients/ needs and, hence, is
simpler/faster to write, review, and integrate, but is likely to
duplicate some clients/ code, requiring more work down the road. Pick
your poison.

I know Christos gave (A) a +1, but I do not think the Project can
dictate which option you must implement: If you want to do (B), you
should be allowed to do it! However, your (B) implementation ought to be
reviewed as (B) then -- accounting for both clients/ and servers/ needs,
to the extent we know or can predict them. IMO, you should integrate the
new Agent class with at least one Client and at least one Server before
posting your Agent for review.

The other critical concern was correctly raised by Henrik (it is so nice
to have more than one person reviewing this stuff!): It is wrong to
write a TcpAgent class. What we need is an Agent class that does not
know whether its connection is a raw TCP connection, an SSL-encrypted
TCP connection, a UDP stream, or something else.

Today, we do not have the appropriate "ConnLayer" class (for the lack of
a better name) to encapsulate those transport layering differences and
nest different layers (the existing Connection class is too low-level
for that), but your Agent will need ConnLayer. You can start by using
the existing Connection as long as your code can handle write-needs-read
and read-needs-write complications.

This second concern is orthogonal to the A vs. B choice above.

Cheers,

Alex.
Received on Fri Jan 24 2014 - 17:45:19 MST