[squid-dev] [PATCH] Bug 4430 Squid crashes on shutdown while cleaning up idle ICAP connections.

[Alex Rousskov]

On 02/29/2016 07:36 PM, Amos Jeffries wrote:

> Please split the IndependentRunner and ConnStateData parts out of the
> main patch. It is unrelated to the ICAP bug specifics.

The IndependentRunner parts are fixing the RR API abuse introduced by
the ConnStateData changes while solving pretty much the same bug.
IndependentRunner is needed by both ConnStateData and ICAP code.
Separating these changes would only increase overhead IMO (but can be
done if you insist, of course).


>  I am a little worried about the virtual change to finishedShutdown(). I
> have found myself several times already mistakenly implementing that
> hook in child Runners. The lack of virtual was what made me take a
> second look and fix my design bug.

>  If we go ahead with this I would kind of like some other mechanism for
> IndependentRunner to use to avoid "delete this" that requires a bit more
> dev thought and work to make use of than simply overridding the virtual
> method.

We will make accidentally overriding finishedShutdown() impossible in
the next version of the patch.



> On the other hand;
>  ConnStateData being a Runner situation was only intended to be a
> temporary middle-term workaround until we had a global list of client
> connections workingproperly and the comm_exit was itself turned into a
> proper Runner.

IndependentRunner simplifies any future migration of shutdown-handling
responsibilities. Our ConnStateData changes help you replace your
temporary workaround with a long-term solution (whatever it is).

IndependentRunner is needed if we have any shutdown handlers that cannot
be deleted by the RR registry. I expect the number of such handlers
increase rather than go to zero.


> On the other hand;
>  comm_exit action still needs to be made into a Runner.

I doubt the end of Comm service should be [a designated RR event]. Like
memory operations, Comm service should be provided when somebody needs
it, no matter when that happens. We are obviously not there yet, but
that is the design I recommend moving towards instead of trying to time
Comm exit "just right" and make all Comm users responsible for handling
a yet another special event.


Thank you,

Alex.
P.S. I wish you were as thorough when reviewing the changes that first
introduced RR abuse for short-lived objects like ConnStateData. Oh, wait...