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