[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.
> 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: SQUID307-half-closed-reader-squid4-t10.patch
Type: text/x-patch
Size: 38541 bytes
Desc: not available
URL: <http://lists.squid-cache.org/pipermail/squid-dev/attachments/20170626/ff976cb0/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: SQUID307-half-closed-reader-squid3.5-t10.patch
Type: text/x-patch
Size: 41825 bytes
Desc: not available
URL: <http://lists.squid-cache.org/pipermail/squid-dev/attachments/20170626/ff976cb0/attachment-0003.bin>