Re: std::vector on-exit crashes

On 04/08/2014 09:28 AM, Tsantilas Christos wrote:

> Alex analysis for the crashes is correct. However I believe that we
> should try fixing the crashes, not be back to squid Vector. Or even if
> we go back to squid Vector, we still need to fix this problem.
> The Squid Vector, just hides the problem does not solve it.

Hi Christos,

    I agree. Moreover, I do not think anybody is calling for the
std::vector changes to be reverted yet. We just need to understand and
fix them. I think we have been making good progress with both recently.

> The problem has different form in various cases. For the Adaptation
> lists (AccessRules, Services, Groups) we are emptying the std::vectors
> inside the the Adaptation::*::TheConfig objects destructor.
> But the std::vector's destroyed before the Adaptation::Icap::TheConfig
> and Adaptation::Ecap::TheConfig objects. So we are trying to access
> destroyed objects.

Agreed.

> The global objects in C++ normally destroyed in the reverse order they
> created. The Adaptation::*::TheConfig objects created on squid start,
> but the std::vector lists created later when the
> adaptation::All(Rules|Services|Groups)() method called. On squid exit,
> the std::vector lists destroyed first.
>
> I am attaching a patch which solves this problem. This patch creates
> small temporary object classes to store the std::vector lists for
> Rules, Services and Groups, which are responsible to empty and release
> the lists on shutdown.
> These lists are not emptied from Adaptation::Config
> (Adaptation::*::TheConfig objects) any more, and a new method the
> Adaptation::Config::Clear() added to empty these lists on reconfigure.

I am not objecting to your change, but please consider the attached
patch as a much simpler alternative that avoids static destruction
dependency altogether. The proposed commit message is in the patch preamble.

I prefer my version because it is much simpler and because the true/core
problem here is kind of elsewhere: We just do not cleanup properly on
exit(*). This is why reconfiguration works without coredumps but exiting
Squid creates problems (kind of). That is a different problem to solve
IMO...

If you want to polish your patch instead, please briefly document the
new classes and methods to comply with code style rules! You should
probably also move most static methods manipulating
groups/rules/services into the new classes. For example,
Adaptation::FindGroup() should be moved into GroupsCfg because it is
specific to adaptation groups, right?

Thank you,

Alex.
(*) There is a bunch of on-exit cleanup code in main.cc that is
currently disabled that should be polished and re-enabled, at least
partially, but we have much bigger problems with memory cleanup during
reconfiguration that we need to address. Patches pending.