Hi there, I have committed the BodyPipe changes described below (with a few modifications) to the squid3-icap branch. Using the new BodyPipe class, I was able to eliminate many ICAP-related classes and code complexities, not to mention a few memory leaks and bugs.
I will be working on a few remaning XXXs next week, but please test the current code if you can, especially if you are using ICAP. **WARNING** The committed changes are significant and probably introduce new bugs! The code passed some Web Polygraph and manual surfing tests, with and without ICAP. FTP-specific changes are untested, but the old code probably did not work well with ICAP either. Please test and submit bug reports. Thank you, Alex. On Tue, 2007-01-30 at 14:33 -0700, Alex Rousskov wrote: > 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. >