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

[Amos Jeffries]

On 1/03/2016 9:31 a.m., Christos Tsantilas wrote:
> Hi all,
> 
>   Squid crashes on shutdown with the following backtrace:
> (gdb) bt
> #0  0x00000000007138d8 in commSetConnTimeout(RefCount<Comm::Connection>
> const&, int, RefCount<AsyncCall>&) ()
> #1  0x0000000000510ddf in
> commUnsetConnTimeout(RefCount<Comm::Connection> const&) ()
> #2  0x00000000006104a4 in
> IdleConnList::clearHandlers(RefCount<Comm::Connection> const&) ()
> #3  0x000000000041226b in IdleConnList::closeN(unsigned long) ()
> #4  0x0000000000412bf9 in IdleConnList::~IdleConnList() ()
> #5  0x0000000000617e9b in Adaptation::Icap::ServiceRep::~ServiceRep() ()
> #6  0x0000000000618579 in Adaptation::Icap::ServiceRep::~ServiceRep() ()
> #7  0x000000000060e79c in Adaptation::DetachServices() ()
> #8  0x0000000000606da0 in Adaptation::Config::freeService() ()
> #9  0x0000000000606e4e in Adaptation::Config::~Config() ()
> #10 0x001237f3e8664e29 in __run_exit_handlers (status=0,
> listp=0x7f3e5c9cf5a8 <__exit_funcs>,
>     run_list_atexit=run_list_atexit at entry=true) at exit.c:82
> 
> 
> The global Adaptation::Icap::TheConfig object is automatically
> destroyed when Squid exits. Its destructor destroys Icap::ServiceRep
> objects that, in turn, close all open connections in the idle
> connections pool. Since this happens after comm_exit has destroyed all
> Comm structures associated with those connections, Squid crases.
> 
> I am attaching two patches. A simple  patch
> (Squid-Segfault-on-Shutdown-t2.patch) which I believe it should applied
> to squid-3.5, but also I am posting a second preview patch
> (Squid-Segfault-on-Shutdown-preview-trunk-t4.patch) with a possible long
> term solution.
> 
> I am suggesting to commit for now the squid3.5 patch while we are
> discussing a long term solution for this or future similar problems for
> trunk.
> 

Okay. +1 on that 3.5 patch going in.

For the other:

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


On the one hand;
 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. Making it easier to use IndependentRunner than to duplicate its
purpose by accident is a good thing.


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.


On the other hand;
 comm_exit action still needs to be made into a Runner. Which will make
its exact timing less predictable and require us to design some
friendlier behaviour for it in the case of still-open FD.
 - for a long-term solution we should aim at that happening sooner
rather than later.
 - this does not conflict with the need to fix the ICAP idle list and
many other FD uses.


I will do a patch audit after your feedback on the above.

Amos