[squid-dev] [PATCH] Reduce "!Comm::MonitorsRead(serverConnection->fd)" assertions.

[Alex Rousskov]

On 06/21/2017 08:24 AM, Amos Jeffries wrote:
> On 21/06/17 23:40, Christos Tsantilas wrote:
>> * Protect Squid Client classes from new requests that compete with
>> ongoing pinned connection use and
>> * resume dealing with new requests when those Client classes are done
>> using the pinned connection.


> in src/FwdState.cc
> 
> * Comm::ConnectionPointer() does not need to be passed nullptr
> parameter, there is a default constructor.

... but please continue to pass it to emphasize that we want to send a
nil pointer to the connection (rather than some "default" answer).


> in src/client_side.cc:
> 
> * regarding the first XXX "Closing pinned conn is too harsh"
>  - the entire point of pinning is to make the transports on both client
> and server connections share their fates.

What you describe is more of a side effect rather than the "entire
point". For the record, pinning has two primary goals:

1. Allow correct server connection reuse (with the pinning client).
2. Prevent incorrect server connection reuse (with other clients).

The two connections certainly "share fate" to some extent, but that fate
sharing does not imply that a closure of one connection must always
trigger an "immediate" closure of the other connection.


> Consider that to do a pinning means something outside Squid (the
> "application layer") is depending on end-to-end semantics (effectively,
> as far as Squid is concerned anyway) and guarantees on the transport it
> thinks it is using. There is no way for Squid to know fully what is
> being passed at those higher levels to send the correct end signal. The
> only available signal we have is to close the things that _are_ known
> about, right up to the two TCP connections which were pinned together.

What you say is true for blind tunnels but false for HTTP connection
pairs where Squid has (and uses!) HTTP mechanisms to control TCP
connection lifetimes. You could argue that MitM attacks break some
clients by changing effective server connection lifetime, and I would
agree with that, but once Squid is told (to attack) to use HTTP, it
should use HTTP signals for HTTP connection lifetimes (subject to
satisfying the primary pinning goals listed above, of course).


> in src/client_side.h:
> 
> * I predict we are going to see Coverity complaints about
> PinnedIdleContext missing move semantics.

PinnedIdleContext already implements move optimizations (implicitly).
See "Implicitly-declared move constructor" at

    http://en.cppreference.com/w/cpp/language/move_constructor


> It has instances initialized
> on the stack and then passed around by-value as if it were a
> smart-pointer

The above "as if" conjunction does not compute for me: A lot of things
are initialized on the stack and then passed around by value that are
not smart pointers. For example, "int", "double", and "struct timeval".


>  - the options here are either to pass by-reference with & operator

I certainly agree that we should pass PinnedIdleContext by reference
where possible. If there are places not doing that in the patch, we
should change them. I do not know of such places, but I could have
missed them, of course.

Please note that asynchronous calls often require by-value parameters
because of how I wrote our AsyncCall parameter storing/type guessing code:

> src/base/AsyncJobCalls.h:139:1: no known conversion for argument 2 from ‘void (ConnStateData::*)(const ConnStateData::PinnedIdleContext&)’ to ‘void (ConnStateData::*)(ConnStateData::PinnedIdleContext)’

I suspect we can remove that limitation, especially in the C++11 world,
but doing so is outside this project scope.


> or to implement move constructor and assignment operator for the class>  - it might be worthwhile implementing as a
> std::tuple<aPointer,bPointer> to get the operators implicitly.

PinnedIdleContext already got all those operators implicitly AFAICT.


HTH,

Alex.