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
