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