On 09/09/2012 08:12 PM, Amos Jeffries wrote: >> sön 2012-09-09 klockan 21:34 +1200 skrev Amos Jeffries: >> >>> Henrik and I seem to disagree on this being a good idea being plugged >>> into BodyPipe. But as I see it BodyPipe is where Upgrade, WebSockets and >>> SSL-Bump should be happening in a node which internally "receives" the >>> tunnel connection and acts like a ConnStateData reading out of the >>> tunnel.
> I'm imagining a setup of: > > client TCP -> ConnStateData(BodyPipeProducer) -> Bump(BodyPipeReader, > 'fake' ConnStateData(BodyPipeProducer) ) -> BodyPipe -> server-side > > so that: > * the bumped CONNECT tunnel can be the source of a whole pipeline of > requests (HTTPS, etc) > * the original ConnStateData->Bump section of code retains all the > CONNECT semantics down to and including terminate 'server' connection > (aka Bump Node destructs) on end of CONNECT independent of what Bump -> > server-side does with the server-side connection. > * when receiving a CONNECT we detect the right type of streams reader > to handle the data and allocate it: > ** the Bump node could be Upgrade:TLS, Upgrade:WebSockets, > Upgrade:HTTP/2, SSL-bump, pass-thru tunnel or some future protocol FWIW, a "processing chain" design like that was one of the two primary alternatives discussed when Squid3 was born. I agree that this design is attractive for many reasons. I am guessing that ClientStreams were an attempt to implement it (but we need to do much better than that!). With ClientStream and adaptation chains lessons in mind, I would be strongly opposed to making BodyPipe a _central_ piece of a processing chain coordination. BodyPipe should be used for low-level shoveling of opaque bytes within the chain, but there is more high-level semantics that a message processing chain must handle and BodyPipe should not. New "message processing stream" and "message processing stream node" classes are needed if we want to build such chains, with the existing ClientStream, StoreClient, and Adaptation code upgraded to use that new API. This is Squid v4.0 stuff IMO... Personally, I would welcome efforts in that direction, and I am glad you have a very interested client pushing you there. However, this monumental work should not become a precondition for fixing this isolated bug, IMO. Isolating [secure] peer connection establishment code into a class shared by tunnel.cc and forward.cc code will already make any future chaining improvements easier. Let's view this as a single step in the right direction. I hope you do not insist on us running the whole marathon within this bug scope :-). Cheers, Alex.