Folks, Executive summary: I am trying to fix several bugs and complaints related to your favorite class, the BodyReader. The changes I would like to make are significant, so I decided to post them here first. Please see the attached BodyPipe.h sketch. I will proceed with these changes unless there are violent objections or better ideas. The email below documents my concerns and explains the rationale behind BodyPipe.
Current state: The BodyReader class does not seem to read a body. It helps to move the body content from the body producer to the consumer. The class is essentially a collection of loosely coupled function pointers with protected and bare data attached to them. The class calls consumer-side functions on behalf of the producer (or vice versa), while updating the body size state. BodyReader refcounting was meant to communicate abort conditions. Here are a few related/intermingled reasons behind BodyPipe, the replacement design I am implementing (see the attached sketch). A) The reason I want to get away from the current "lose ends" approach with function pointers is to provide a more clear, easier to follow implementation (IMO). I want classes like ConnStateData or ICAPClientReqmodPrecache to inherit from BodyProducer and just implement the two or three required methods. No wrappers or bare data pointers at the high level, where all the changes and enhancements are taking place. B) The reason I want explicit provider and consumer pointers inside BodyPipe is to be able to abort either side if the other side is having trouble. The current idea of using refcounting to abort something does not and cannot work (if there are two refcounted pointers, the second pointer will not know that the first has been set to NULL). I also want joint ownership of the BodyReader so that the producer and the consumer know that they must clear/detach themselves before forgetting about the pipe. Refcounting will just help to destroy the abandoned pipe. C1) The reason I want asynchronous calls is to separate producer from consumer and break the call-me-back-while-I-am-calling-you loops. I believe that at least the abortedSize assertion (bug #1862) is caused, in part, by such loops. C2) Another reason for separating producer and consumer is to allow the producer to disconnect when done, leaving a functional object (with the remaining/last body bytes) behind. This situation happens when, for example, an ICAP transaction is finished but the HttpStateData is still writing the body to the origin server. Again, please take a look at the attached sketch. I have seen this before: While sketching the design, I realized that what I want is very similar to MsgPipe, a class used by ICAP-related code to shovel HTTP messages between Squid core and an ICAP transaction. If you look at MsgPipe.cc and ignore the low-level debugging macros, you will see that there is virtually nothing there. The class simply converts notifications from one pipe end to events and then calls the other end when the event fires. I do not plan on reusing MsgPipe itself because its simple design does not allow one object to be the end of two pipes -- there would be no way to know which pipe the messages are coming from. I do not want to complicate MsgPipe and its users to support multi-pipe ends. I think it would be better to have a very similar but distinct BodyPipe class, with method names specific to the problem at hand and extra code to handle body size calculations. The design duplication will be there, but code duplication should be negligible. I am worried that I see every problem as a nail though, which is another motivation for this message. Again, if you have better ideas on how to fix BodyReader-related problems, please stop me and contribute! Thank you, Alex.
// interface for those who want to provide body content to BodyPipe class BodyProvider { public: virtual BodyProvider() {} virtual provideMoreBody(BodyPipe *pipe) = 0; virtual noteBodyConsumerAbort(BodyPipe *pipe) = 0; }; // interface for those who want to consume body content from BodyPipe class BodyConsumer { public: virtual BodyConsumer() {} virtual consumeMoreBody(BodyPipe *pipe) = 0; virtual noteBodyProviderAbort(BodyPipe *pipe) = 0; }; // Connects those who produce message body content with those who // consume it. For example, connects ConnStateData and FtpStateData OR // ICAPClientReqmodPrecache and HttpStateData. class BodyPipe: public RefCountable { public: typedef RefCount<BodyPipe> Pointer; public: BodyPipe(size_t aLength, Provider *aProvider); ~BodyPipe(); // clear provider and consumer first // called by providers void clearProvider(bool keepPiping); size_t spaceSize() const; size_t putMoreData(const char *buf, size_t size); // called by consumers void setConsumer(Consumer *aConsumer); void clearConsumer(bool keepPiping); size_t dataSize() const; size_t getMoreData(MemBuf &buf); private: const size_t theLength; // expected content length Provider *theProvider; // content provider, if any Provider *theConsumer; // content consumer, if any size_t thePutSize; // ever-increasing total size_t theGetSize; // ever-increasing total MemBuf theBuf; // provided but not yet consumed content, if any CBDATA_CLASS2(BodyPipe); };