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

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.

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

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

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.

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.

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

> +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?

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

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

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

> + * 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]/

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"

> + 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?

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

> + 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?

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

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

Thank you,

Alex.
Received on Fri Jan 03 2014 - 19:16:18 MST