Squid3 BodyReader changes

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.