Re: std::vector on-exit crashes

On 04/09/2014 02:17 AM, Alex Rousskov wrote:
> On 04/08/2014 09:28 AM, Tsantilas Christos wrote:
>> 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 agree that your patch is simpler, and better.

What still I do not like in adaptation::Config code is to release inside
adaptation::Config (adaptation/Config.cc) objects stored in
adaptation/AccessRules.cc for example.
But I do not believe that worth to spent our time for this now.

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

OK, I agree.

>
>
> 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.
>
Received on Wed Apr 09 2014 - 15:00:48 MDT