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

[Christos Tsantilas]

I applied the patch to squid-5 as r15226.

The applied patch includes the latest fixes requested by Alex.  The 
HttpRequest reference passed as parameter to the 
ConnStateData::pinConnection and also the RefCount class modified to 
assert on dereference operator when the pointer is null.


I am also attaching the patches for squid-3.5 and squid-4.
The squid-3.5 patch passes the HttpRequest::Pointer as parameter to the 
ConnStateData::pinConnection method.





Στις 23/06/2017 12:53 μμ, ο Christos Tsantilas έγραψε:

A new patch

Στις 21/06/2017 08:07 μμ, ο Alex Rousskov έγραψε:
On 06/21/2017 05:40 AM, Christos Tsantilas wrote: I suggest the 
following one or two polishing touches:


1. Merge pinConnection() and pinNewConnection() by returning from
the method if there is nothing to do, with a debugs() line printing 
pinServer. The merged method would be called pinConnection(), of course.


OK, done.



2. *If* the request object is actually always there, then change the 
pinConnection() parameter to an Http::Request reference (and change 
callers to dereference their request pointers).



I changed the "HttpRequest *" parameter to HttpRequest::Pointer
reference, I think this is the best.
I think currently the HttpRequest::Pointer is always not NULL, we can
add a "Must()" check if required, but HttpRequest object is not critical
for pinning operation.



* 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.



Currently when the squid-server connection is closed, we are always
closing the client-squid connection. For HTTP and FTP is not always the
best option.
On the other side if the client-squid connection closed the squid-server
side may want to continue download and store to cache an object, or may
want to cleanup and close properly with the server.

So I let this comment to the patch.
We can discuss it more when someone try to solve pinned connections
closing problems.


in src/client_side.h:

* I predict we are going to see Coverity complaints about 
PinnedIdleContext missing move semantics.


The Coverity should not complain about.
Can we test with Coverity before apply the patch, or can we apply and if
there are problems implement a move constructor?



- 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.



The ConnStateData::notePinnedConnectionBecameIdle and 
ConnStateData::httpsPeeked are passing by value the PinnedIdleContext.

Because they are AsyncCalls we can not do something else.

However the httpsPeeked calls notePinnedConnectionBecomeIdle (and pass 
PinnedIdleContext object by value).


Maybe we can move the notePinnedConnectionBecomeIdle code to a 
pinnedConnectionBecomeIdle(PinnedIdleContext &) method and call this 
method from httpsPeeked and notePinnedConnectionBecomeIdle methods.


Reduce "!Comm::MonitorsRead(serverConnection->fd)" assertions.

* 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

=== modified file 'src/FwdState.cc'
--- src/FwdState.cc	2017-05-07 20:16:59 +
+++ 

[squid-dev] Jenkins build is back to normal : template-full-matrix » clang,d-fedora-23 #395

[noc]

See 


___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

[squid-dev] Build failed in Jenkins: template-full-matrix » clang,d-fedora-23 #394

[noc]

See 


--
[...truncated 280.12 KB...]
libtool: link: ( cd ".libs" && rm -f "libntlmauth.la" && ln -s 
"../libntlmauth.la" "libntlmauth.la" )
make[3]: Leaving directory 
'
make[3]: Entering directory 
'
depbase=`echo base64.lo | sed 's|[^/]*$|.deps/&|;s|\.lo$||'`;\
/bin/sh ../libtool  --tag=CC   --mode=compile ccache clang -DHAVE_CONFIG_H   
-I../../.. -I../../../include -I../../../lib -I../../../src -I../include 
-Werror -Qunused-arguments  -D_REENTRANT -g -O2 -MT base64.lo -MD -MP -MF 
$depbase.Tpo -c -o base64.lo ../../../lib/base64.c &&\
mv -f $depbase.Tpo $depbase.Plo
depbase=`echo charset.lo | sed 's|[^/]*$|.deps/&|;s|\.lo$||'`;\
/bin/sh ../libtool  --tag=CC   --mode=compile ccache clang -DHAVE_CONFIG_H   
-I../../.. -I../../../include -I../../../lib -I../../../src -I../include 
-Werror -Qunused-arguments  -D_REENTRANT -g -O2 -MT charset.lo -MD -MP -MF 
$depbase.Tpo -c -o charset.lo ../../../lib/charset.c &&\
mv -f $depbase.Tpo $depbase.Plo
depbase=`echo html_quote.lo | sed 's|[^/]*$|.deps/&|;s|\.lo$||'`;\
/bin/sh ../libtool  --tag=CC   --mode=compile ccache clang -DHAVE_CONFIG_H   
-I../../.. -I../../../include -I../../../lib -I../../../src -I../include 
-Werror -Qunused-arguments  -D_REENTRANT -g -O2 -MT html_quote.lo -MD -MP -MF 
$depbase.Tpo -c -o html_quote.lo ../../../lib/html_quote.c &&\
mv -f $depbase.Tpo $depbase.Plo
depbase=`echo md5.lo | sed 's|[^/]*$|.deps/&|;s|\.lo$||'`;\
/bin/sh ../libtool  --tag=CC   --mode=compile ccache clang -DHAVE_CONFIG_H   
-I../../.. -I../../../include -I../../../lib -I../../../src -I../include 
-Werror -Qunused-arguments  -D_REENTRANT -g -O2 -MT md5.lo -MD -MP -MF 
$depbase.Tpo -c -o md5.lo ../../../lib/md5.c &&\
mv -f $depbase.Tpo $depbase.Plo
libtool: compile:  ccache clang -DHAVE_CONFIG_H -I../../.. -I../../../include 
-I../../../lib -I../../../src -I../include -Werror -Qunused-arguments 
-D_REENTRANT -g -O2 -MT base64.lo -MD -MP -MF .deps/base64.Tpo -c 
../../../lib/base64.c  -fPIC -DPIC -o .libs/base64.o
libtool: compile:  ccache clang -DHAVE_CONFIG_H -I../../.. -I../../../include 
-I../../../lib -I../../../src -I../include -Werror -Qunused-arguments 
-D_REENTRANT -g -O2 -MT md5.lo -MD -MP -MF .deps/md5.Tpo -c ../../../lib/md5.c  
-fPIC -DPIC -o .libs/md5.o
libtool: compile:  ccache clang -DHAVE_CONFIG_H -I../../.. -I../../../include 
-I../../../lib -I../../../src -I../include -Werror -Qunused-arguments 
-D_REENTRANT -g -O2 -MT html_quote.lo -MD -MP -MF .deps/html_quote.Tpo -c 
../../../lib/html_quote.c  -fPIC -DPIC -o .libs/html_quote.o
libtool: compile:  ccache clang -DHAVE_CONFIG_H -I../../.. -I../../../include 
-I../../../lib -I../../../src -I../include -Werror -Qunused-arguments 
-D_REENTRANT -g -O2 -MT charset.lo -MD -MP -MF .deps/charset.Tpo -c 
../../../lib/charset.c  -fPIC -DPIC -o .libs/charset.o
libtool: compile:  ccache clang -DHAVE_CONFIG_H -I../../.. -I../../../include 
-I../../../lib -I../../../src -I../include -Werror -Qunused-arguments 
-D_REENTRANT -g -O2 -MT base64.lo -MD -MP -MF .deps/base64.Tpo -c 
../../../lib/base64.c -o base64.o >/dev/null 2>&1
libtool: compile:  ccache clang -DHAVE_CONFIG_H -I../../.. -I../../../include 
-I../../../lib -I../../../src -I../include -Werror -Qunused-arguments 
-D_REENTRANT -g -O2 -MT html_quote.lo -MD -MP -MF .deps/html_quote.Tpo -c 
../../../lib/html_quote.c -o html_quote.o >/dev/null 2>&1
libtool: compile:  ccache clang -DHAVE_CONFIG_H -I../../.. -I../../../include 
-I../../../lib -I../../../src -I../include -Werror -Qunused-arguments 
-D_REENTRANT -g -O2 -MT md5.lo -MD -MP -MF .deps/md5.Tpo -c ../../../lib/md5.c 
-o md5.o >/dev/null 2>&1
libtool: compile:  ccache clang -DHAVE_CONFIG_H -I../../.. -I../../../include 
-I../../../lib -I../../../src -I../include -Werror -Qunused-arguments 
-D_REENTRANT -g -O2 -MT charset.lo -MD -MP -MF .deps/charset.Tpo -c 
../../../lib/charset.c -o charset.o >/dev/null 2>&1
depbase=`echo rfc1738.lo | sed 's|[^/]*$|.deps/&|;s|\.lo$||'`;\
/bin/sh ../libtool  --tag=CC   --mode=compile ccache clang -DHAVE_CONFIG_H   
-I../../.. -I../../../include -I../../../lib -I../../../src -I../include 
-Werror -Qunused-arguments  -D_REENTRANT -g -O2 -MT rfc1738.lo -MD -MP -MF 
$depbase.Tpo -c -o rfc1738.lo ../../../lib/rfc1738.c &&\
mv -f $depbase.Tpo $depbase.Plo
depbase=`echo rfc2617.lo | sed 's|[^/]*$|.deps/&|;s|\.lo$||'`;\
/bin/sh ../libtool  --tag=CC   --mode=compile ccache clang -DHAVE_CONFIG_H   
-I../../.. -I../../../include -I../../../lib -I../../../src -I../include 
-Werror -Qunused-arguments  -D_REENTRANT -g -O2