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
_______________________________________________
squid-dev mailing list
[email protected]
http://lists.squid-cache.org/listinfo/squid-dev
