Re: std::vector on-exit crashes

The problem still exist in trunk..
If no objection I will apply latest Alex's patch to trunk....

Regards,
   Christos

On 04/09/2014 02:17 AM, Alex Rousskov wrote:
> 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.
>
Received on Fri May 09 2014 - 09:26:39 MDT