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

[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-t9.patch
Type: text/x-patch
Size: 38532 bytes
Desc: not available
URL: <http://lists.squid-cache.org/pipermail/squid-dev/attachments/20170623/fc4db0e3/attachment-0001.bin>