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

On 22/01/2014 6:45 p.m., 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.
>

Done.
<snip naming and scope changes discussion, see other response>
>
>>>> +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).
>
> Let's call this method stopReadingXXX() so that it is clear that any
> caller is "doing it wrong" and to discourage use.

Done.

>
>> +bool
>> +Comm::TcpReceiver::maybeMakeSpaceAvailable()
>> +{
>> + /*
>> + * why <2? Because delayAwareRead() won't actually read if
>> + * you ask it to read 1 byte. The delayed read(2) 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.
>> + */
>> +
>
> That comment does not match the code. The method is not scheduling any
> reads, it is just allocating buffer space. I would just add a simpler
> comment like this:
>
> // XXX: <2 is probably for delayAwareRead() in callers to work, as
> // discussed in HttpStateData::maybeReadVirginBodymethod().
>

Er. That method is erased now. The comment (and code) was *moved* into
the spot you are talking about.
I've done a slight change to the comment to make it mention how the
read(2) is related to this methods code.

>
>> +Comm::TcpReceiver::readIoHandler(const CommIoCbParams &io)
>
> Drop the "Io" part because "read" already implies "Io"?
>

Done.

>
>> + assert(Comm::IsConnOpen(tcp));
>> + assert(io.conn->fd == tcp->fd);
>
> These should probably be Must()s since we are running under job protection.
>

Okay. Done.

>
>> + /*
>> + * Don't reset the timeout value here. The timeout value will be
>> + * set to Config.Timeout.request by httpAccept() and
>> + * clientWriteComplete(), and should apply to the request as a
>> + * whole, not individual read() calls. Plus, it breaks our
>> + * lame half-close detection
>> + */
>
>
> Too specific to the servers/ code (given your desire to use this for
> clients/, servers/, and even adaptation services).

Polished up.

>> + if (size < 0) {
>> + if (!ignoreErrno(xerrno)) {
>> + debugs(5, 2, tcp << " read failure: " << xstrerr(xerrno));
>> + return true;
>> + } else if (!inBuf.hasContent()) {
>> + debugs(5, 2, tcp << ": no data to process (" << xstrerr(xerrno) << ")");
>> + }
>> + }
>> +
>> + return false;
>
>
> The ignoreErrno() == true case above does not work well for the caller:
> If there was a minor error, the caller should keep reading, not
> re-processing the old inBuf content that has not changed. More on that
> further below (**).

Added calls to try growing the buffer and read again without processing.

For now I have also picked to treat the full buffer case same as 0-sized
read. With a call to the maybeFinishedWithTcp() and setting up
half-closed monitor to watch for full closure.

>
>> + if (readWasError(io.flag, io.size, io.xerrno)) {
>> + noteTcpReadError(io.xerrno);
>> + io.conn->close();
>> + return;
>> + }
>
> The io.conn->close() part should probably be inside noteTcpReadError(),
> which kids may overwrite and call. More on that futher below (**).

Maybe. I left it out to ensure that no matter when the child called its
parents function the connection would only be closed after the calls all
return. Makes it harder for them to step on each others feets.

>
>> + // if the connection is still possibly sending
>> + // the child class may be able to stop immediately
>> + if (const char *reason = maybeFinishedWithTcp()) {
>> + stopSending(reason); // will close connection
>> + return;
>> + }
>
> Missing "not" somewhere in the top comment?
>

No. just bad phrasing. Polished it up.

>
>> + if (io.flag == COMM_OK) {
> ...
>> + } else if (io.size == 0) {
>> + debugs(5, 5, io.conn << " closed?");
>> + stopReceiving("zero sized read(2) result");
> ...
>> + /* It might be half-closed, we can't tell */
>> + fd_table[io.conn->fd].flags.socket_eof = true;
>> + commMarkHalfClosed(io.conn->fd);
>> + fd_note(io.conn->fd, "half-closed");
>> + }
>> + }
>> +
>> + bool mayReadMore = true;
>> + // pass handling on to child instance code
>> + if (inBuf.hasContent())
>> + mayReadMore = processReadBuffer(inBuf);
>
>
> There seems to be a couple of logic bugs here:
>

FWIW this was cut-n-paste code for the most part. So these logic errors
are what exist in trunk right now.

> * If we read zero bytes, we should call the parser again (this time with
> an eof flag), even if the TCP connection is closed now. Parse-after-eof
> is necessary because a zero-read may indicate the end of the message,
> etc. The above code does not do that. This may not be very kosher for a
> server and is wrong for a client.

The 0-size read case calls maybeFinishedWithTcp(), where the child class
is responsible for any such incompletely buffered message handling. I
have adjusted the documentation in indicate that.

>
> * If we read zero bytes, we should not be reading anymore but the code
> continues with reading-specific checks/calls.

Fixed now with the shuffling.

>
> (**): Your readWasError() method may help to address many issues raised
> above, actually. That method is currently half-baked. It detects errors,
> including the ones we should ignore, but the caller still has to
> remember check the io.flag and deal with other special conditions. What
> you probably want is to move readWasError() code inside
> Comm::TcpReceiver::readIoHandler() but have readIoHandler() call the
> appropriate handling methods, acting as a dispatcher. Here is a sketch:
>
> Comm::TcpReceiver::readIoHandler() {
> ...
> if (bad error)
> handleReadError(...);
> else if (ignored error)
> ; // do nothing
> else if (size == 0)
> handleReadEof();
> else
> handleReadData(...)
> ...
> }
>
> Some of the above methods can be implemented in TcpReceiver, some in
> kids, some on both layers.
>

I've re-written this as a series of if-else statements.

>
>>>> + // 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.
>
> I hope the cbdata problem is resolved now, but please let me know if you
> need my help with any aftershocks.

Two days and I have not found any. :-)

>
>
>> + /// initialize the TCP connection event handlers
>> + /// close(2) callback etc.
>> + void tcpConnectionInit();
>
> * The "tcp" prefix seems redundant -- the whole class is about receiving
> over a TCP connection and there are no other connections around.

Removed.

>
> * Have you considered passing the connection to this method instead of
> the constructor that cannot really do anything with the connection? I
> have not seen the callers yet, but it feels like that would be a cleaner
> design.
>

It seems clean enough in the callers as well. Trying it now.

>
>> + void releaseTcpConnection(const char *reason);
>
> Same problem with the "tcp" prefix.
>

Done.

>
>> +void
>> +Comm::TcpReceiver::releaseTcpConnection(const char *reason)
>> +{
>> + // Used by server-side to release the connection before
>> + // storing it in Pconn pool
>> + comm_remove_close_handler(tcp->fd, closed_);
>> + closed_->cancel(reason);
>> + stopReading();
>> +}
>
> * What about the HTTP Server (client_side*.*) agent? It has to maintain
> a connection pool as well, right? If the code is really specific to
> clients/, why place it here?

No the HTTP server has exactly one connection. I think you are possibly
mistaking pconn (FD pool) for the pipeline list of active transactions.
Or the ICAP pconn pool, which "side" I've not gone near yet.

Replaced the "server-side" term anyway.

>
> * Hm.. Earlier you promised that stopReading() is a hack exclusively for
> TCP tunnels. Here it seems to be used by ordinary clients/.
>

Right. Bit of premature optimization there using the hack function for
the read handler bit. Replaced it for clarity.

>
>> + closed_->cancel("graceful close");
>> + tcp->close();
>
> It is best to be specific in those cases because there are many graceful
> closures but we want to mark this one (for debugging/triage). I suggest
> s/graceful close/Comm::TcpReceiver::stopSending/, especially since we
> may be closing due to an error.

Okay. Done.

>
>> +/* This is a handler normally called by comm_close() */
>> +void
>> +Comm::TcpReceiver::tcpConnectionClosed(const CommCloseCbParams &io)
>
> I think it is best to call all Comm handlers handle*(). This one could
> be handleConnectionClose() or something like that.

Done.

Ran out of time to test the new readHandler behaviour or add the
Comm::Write API wrapper. Next patch to come after those are done.

Amos
Received on Thu Jan 23 2014 - 13:23:58 MST