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

On 22/01/2014 10:32 p.m., Henrik Nordström wrote:
> tis 2014-01-21 klockan 22:45 -0700 skrev Alex Rousskov:
>
>> 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?
>
> I haven't looked in detail at what is being done, but also keep in mind
> that if you layer TLS ontop of a TCP connection then the TLS layer needs
> both read & write events to the TCP connection.
>
> From a code design perspective it should be the same interface for
> application protocol data for both TCP and TLS.
>
> For some food of thought I would throw SCTP into that mix as well, which
> adds another layer ontop of the socket to track which SCTP channel the
> application level connection runs on. So we might have aplication /
> TLS / SCTP channel / SCTP(protocol).
>
> Any design that tries to make application level code (i.e. http/ftp
> protocol handlers etc) needing to be aware of TcpReceiver/Sender is
> plain wrong imho.

Yes. I was loosely thinking of completing this TcpAgent then working on
TlsAgent, etc. with the same interface methods. COAPS and UDP I/O
streams are also on the wishlist as other *Agent types.

With Alex proposal to merge the Comm::Write API into these fully I think
we have a good design to go forward with that can be spread across all
those transport types relatively easily.

>
>> 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.
>
> We should only have channels and listeners at application protocol level
> code (HTTP, ftp, whatever). To most code it does not matter what
> transport these use.
>
> Details of each transport implementation is more free. Not so much need
> to argue on details there. But should not be visible in other code.
>

Precisely. The long-term plan is to have the clients/ and servers/
classes using these FooAgent classes or their children for abstract
access to buffered byte streams instead of dealing with any connection
specifics themselves.

>
>>> It only exists because tunnel.cc takes over the FD. Really nasty (the
>>> dropped data race condition).
>
> yes, CONNECT requests will be a mess. But as above, should take over the
> connection. Any application payload already read need to be either
> handed back to the connection or given to the CONNECT forwarder.
>
>> Let's call this method stopReadingXXX() so that it is clear that any
>> caller is "doing it wrong" and to discourage use.
>
> Yes. What is needed is to stop accepting pipelined requests after a
> CONNECT. Stopping reading is justa brute-force way of accomplishing
> that.
>
>> I do not know or do not remember, sorry. The whole "delay pools cannot
>> deal with single-byte buffers" is a delay pools bug, outside this
>> project scope.
>
> I think it's an ancient parser issue which hopefully is long since
> fixed.
>

Thank you. So we have several possible reasons. At least delay pools is
likely to be around for some time yet. So I'm going to leave this as-is
for the purposes of this patch. It is not exactly harmful to any design
as-is just nasty looking code.

Amos
Received on Thu Jan 23 2014 - 09:34:19 MST