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

[Alex Rousskov]

On 06/23/2017 03:53 AM, Christos Tsantilas wrote:
> Στις 21/06/2017 08:07 μμ, ο Alex Rousskov έγραψε:
>> 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.

I understand your hesitation. Replacing what you think is unused code
with an assertion that it is unused is risky. In this particular case, I
recommend this replacement for v5 (so that we are making progress with
code cleanup) but not for v3.5. Your call.

BTW, the Must() you are talking about should probably be implemented as
an assertion inside the Pointer dereferencing method in RefCount.


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

PinnedIdleContext has an implicit move constructor so there is nothing
to do here IMO. We should not add code unless there are some compelling
reasons for doing so, and no such reasons have been provided so far in
this case.

In general, you can certainly test with Coverity first, but I think that
requiring such manual tests in this context would be too onerous,
especially given the current testing setup. Eventually, such pre-commit
tests of pull requests will be automated.


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

IMO, the increased interface complexity from adding another wrapper
method is not worth saving two smart pointer copies (that may be
optimized away be the compiler).


> +        debugs(33, 3, pinServer << "is already pinned");

I recommend swapping the two streaming elements because pinServer is
going to expand to many tokens, making "is already pinned" info more
difficult to read:

    debugs(33, 3, "already pinned " << pinServer);

This is a trivial change that can be done during commit, of course.


Thank you,

Alex.