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

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. Based on the class name alone, one could
> assume that this class is a job that receives something over TCP. Based
> on the stopSending() and other methods, it is a job that receives and
> sends something over TCP. Please clearly state what you are trying to
> implement so that it becomes possible to check whether your intent
> matches your implementation.
>

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.

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?

>
>> It provides:
>>
>> * data members for referencing a TCP socket and read buffer.
>>
>> * methods for incremental parsing or processing of recieved data.
>>
>> * methods for managing half and fully closing a socket.
>>
>> * separation of socket read() from HTTP/1 parsing and processing logic.
>> It may be re-used for any client-side or server-side TCP socket.
>
> There seems to be some sending-related code as well, but the above does
> not mention it.
>

That is the "managing half and fully closing a socket" bit.

>
>> This patch includes only the Comm::TcpReceiver class and integration
>> with src/comm/. The integration and replacement of client-side code is
>> contained in the larger followup part-2 patch.
>
> Do you plan to use the new classes on the server-side as well? Or is
> that out of [the larger project] scope?

Yes. That is planned to be the part-3 patch. I was having trouble with
multiple inheritence or it would be submitted as well with these pt1/pt2
patches.

See separate email about debugs in CBDATA for the details on that.

>
> TCP-related senders and receivers are not limited to client and server
> sides. Do you plan to use the new classes on the ICAP, DNS, and HTCP
> sides as well.

Yes. For anywhere doing read(2) on a TCP socket.

>
> The answers to the above questions may help you define (and name)
> TcpReceiver correctly and help others understand what this code/change
> is really about.
>
>
>> + /* Ideally this would be setup by the constructor but it involves
>> + * calls to toCbdata() virtual function implemented by the derived class
>> + * so must be run explicitly by that class's constructor.
>> + */
>> +
>
> No, it should either be a part of TcpReceiver::start() method or called
> by kids (during or after their start() method), depending on whether you
> want to support kids that do not start reading and writing immediately
> when the job starts.
>

Okay. Right. Then the function is okay as-is. Changing the comment.

>
>> + /// called when sending has stopped to check if more reads may be required.
>> + virtual int64_t mayNeedToReadMore() const = 0;
>
> What should the kid implementation of this method return? Based on the
> method name and description, I would expect a boolean result so a
> non-boolean result type must be documented.
>

Number of bytes still expected, or -1 if unknown amount. 0 for none
expected.

Updated the comment with return values.

>
>> +void
>> +Comm::TcpReceiver::stopReading()
>> +{
>> + /* NP: This is a hack only needed to allow TunnelStateData
>> + * to take control of a socket despite any scheduled read.
>> + */
>> + if (reading()) {
>> + comm_read_cancel(tcp->fd, reader_);
>> + reader_ = NULL;
>> + }
>> +}
>
> Why is this called a hack?
>

It only exists because tunnel.cc takes over the FD. Really nasty (the
dropped data race condition).

Tunnel should be made into a class that can use ConnStateData API (and
all the related transport encapsulation, accounting, stats, etc) for its
client-facing I/O instead of runnign off with the FD and acting like its
plain TCP.
All this effort duplicating client-side logics in tunnel.cc is very
painful..
Very shortly we will have HTTP/2 and perhapse SCTP etc. per-hop
transports to account for instead of just a plain TCP socket.

> What happens if Comm already read the data but the job does not know
> that yet?

Poof. Dropped when the cancelled AsyncCall pointing to the new I/O
buffer segment gets deleted.

All I have done is shuffle the function into this class. The problem
already exists and I do not see any way we can solve it within
reasonabel scope of this project. So I opted to mark with a comment and
leave it for now.

>
>> + if (inBuf.spaceSize() < 2) {
>
> Please either use !spaceSize() or add a comment to explain why having a
> non-empty buffer is not enough (i.e., why we must have at least two
> bytes of buffer space).
>

This is a delay pools or read(2) limitation apparently. I have no idea
why it had that in the client-side when I started.

But the same check condition was documented in the server-side code thusly:

"
    /*
     * why <2? Because delayAwareRead() won't actually read if
     * you ask it to read 1 byte. The delayed read request
     * just gets re-queued until the client side drains, then
     * the I/O thread hangs. Better to not register any read
     * handler until we get a notification from someone that
     * its okay to read again if the buffer cannot grow.
     */
"

Perhapse it was in client-side for the client delay pools but not
documented properly as such?

>
>> + if (inBuf.spaceSize() < 2) {
> ...
>> + (void)inBuf.space(inBuf.contentSize()*2);
>> + }
>> + return true;
>
> It would be better to return (inBuf.spaceSize() < 2) or the corrected
> condition, in case our attempt to grow the buffer has failed.
>

hasPotentialSpace() is supposed to be taking care of that.
Doing return (inBuf.spaceSize() > 1) anyway.

>
>> + * may return false. The processor is then responsible for ensuring that
>> + * readSomeData() is called when read() are to be resumed.
>> + *
>> + * \retval true if additional read() are acceptible.
>> + * \retval false if read() are to be halted.
>> + */
>> + virtual bool processReadBuffer(MemBuf &) = 0;
>
> s/are to be halted/are to be suspended/
>
> s/are acceptible/should be scheduled [by the caller]/
>

Done.

> Throughout the patch, you use the term read(), usually in plural
> context. It is not clear what that is, especially since there is no such
> class method. If you mean the system call, please use "read(2)s" or
> "read(2) calls", but it is better to rephrase in more context-specific
> terms when possible. For example, in the above context, you can use
> "readSomeData() calls are" instead for "read() are"
>

Okay. Tried to find them all.

>
>> + if (inBuf.hasContent())
>> + mayReadMore = processReadBuffer(inBuf);
>> +
>> + if (!maybeMakeSpaceAvailable()) {
>
> Why try to make more space available if the kid told us we should
> suspend reading? Add mayReadMore precondition to the second if-statement?
>

There was some reason. I have forgotten. Giving it a try under the
current code to see what falls out of the woodwork.

>
>> + /// called when there is an I/O error reading
>> + virtual void noteTcpReadError(int) {}
>
> Please adjust description to clarify why it is OK to ignore this call.
> If it is not OK to ignore this call, make this method pure virtual.
>

Done.

>
>> + MemBuf inBuf;
>
> I assume you cannot convert that to MemBlob at this time because Comm
> does not accept MemBlobs, right? Perhaps add a "TODO: Use MemBlob when
> Comm is ready" comment?
>

We can't convert to bare MemBlob at all here because there appears to be
no API to assign a MemBlob to an SBuf without data copy. But switching
MemBlob to work from memmove() just like MemBuf does now we can use SBuf
API.

Then there is the issue of conversion requiring all the code paths using
MemBuf and be converted in lock-step fashion if we are to avoid
transitional data copies between SBuf and MemBuf.

>
>> + stopReceiving("client zero sized read");
>
>> + return; // wait for the request receiver to finish reading
>
> My understanding is that this code is not meant to be specific to the
> client-side. The above lines are specific to the client-side.
>

Yes. Fixed.

>
>> + // AsyncJob API
>> + virtual bool doneAll() const;
>> + virtual void swanSong();
>> + void tcpConnectionInit();
>> + virtual int64_t mayNeedToReadMore() const = 0;
>> + bool maybeMakeSpaceAvailable();
>> + virtual bool processReadBuffer(MemBuf &) = 0;
>> + virtual void noteTcpReadError(int) {}
>> + void readIoHandler(const CommIoCbParams &io);
>> + virtual const char * maybeFinishedWithTcp() = 0;
>
> Please review all public methods and make each methods that is meant to
> be called by parent or kid classes only protected instead of public. The
> above lists some good candidates, but I have not carefully verified each
> of them and I may have missed some.
>
> In general, if you can make a method protected, do so and if you can
> make a data member private, do so as well. This helps protecting classes
> from invasive calls that should be using different/public interfaces.
>

My workaround for the server-side trouble was to make this
ServerStateData inherit from this TcpReceiver class. Which has trouble
with the 2nd generation inheriting child being the one implementing the
virtual when its a protected/private membet ofr the middle inheritence
class.

Will do the protected changes when I am sufficiently happy that the
CBDATA calls are working properly to avoid the workaround.
 For some reason the failure case in ServerStateData is being reluctant
to appear again.

Amos
Received on Tue Jan 07 2014 - 09:52:47 MST