Re: Squid3 BodyReader changes

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.
>
Received on Wed Feb 14 2007 - 00:36:32 MST