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

[Amos Jeffries]

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.
>
> Replaced primary ConnStateData::pinConnection() calls with a pair of
> pinBusyConnection() and notePinnedConnectionBecameIdle() calls,
> depending on the pinned connection state ("busy" or "idle").
>
> Removed pinConnection() parameters that were not longer used or could be
> computed from the remaining parameters.
>
> Removed ConnStateData::httpsPeeked() code "hiding" the originating
> request and connection peer details while entering the first "idle"
> state. The old (trunk r11880.1.6) bump-server-first code used a pair of
> NULLs because "Intercepted connections do not have requests at the
> connection pinning stage", but that limitation no longer applicable
> because Squid always fakes (when intercepting) or parses (a CONNECT)
> request now, even during SslBump step1.
>
> The added XXX and TODOs are not directly related to this fix. They were
> added to document problems discovered while working on this fix.
>
> In v3.5 code, the same problems manifest as Read.cc
> "fd_table[conn->fd].halfClosedReader != NULL" assertions.
>
> This is a Measurement Factory project
>


Audit:

in src/FwdState.cc

* Comm::ConnectionPointer() does not need to be passed nullptr 
parameter, there is a default constructor.


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.

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.


in src/client_side.h:

* I predict we are going to see Coverity complaints about 
PinnedIdleContext missing move semantics. It has instances initialized 
on the stack and then passed around by-value as if it were a 
smart-pointer rather than a class storing several smart pointers.
  - the options here are either to pass by-reference with & operator, 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.


in src/tests/stub_client_side.cc:

* no need for the pic parameter name in the 
notePinnedConnectionBecameIdle() stub.

Amos