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.
Avoid on-exit crashes when adaptation is enabled.
After trunk r13269 (Vector refactor) destroyed vector objects still have
positive item counts. This exposes use-after-delete bugs. In this particular
case, global adaptation rule/group/service arrays are destructed by global
destruction sequence first and then again by Adaptation::*::TheConfig objects
destructors.
This change avoiding static destruction order dependencies by storing those
global adaptation arrays on heap.
=== modified file 'src/adaptation/AccessRule.cc'
--- src/adaptation/AccessRule.cc 2013-05-13 22:48:23 +0000
+++ src/adaptation/AccessRule.cc 2014-02-20 01:27:36 +0000
@@ -34,42 +34,42 @@ Adaptation::AccessRule::finalize()
g->finalize(); // explicit groups were finalized before rules
AllGroups().push_back(g);
}
}
if (!group()) {
debugs(93, DBG_CRITICAL, "ERROR: Unknown adaptation service or group name: '" <<
groupId << "'"); // TODO: fail on failures
}
}
Adaptation::ServiceGroupPointer
Adaptation::AccessRule::group()
{
return FindGroup(groupId);
}
Adaptation::AccessRules &
Adaptation::AllRules()
{
- static AccessRules TheRules;
- return TheRules;
+ static AccessRules *TheRules = new AccessRules;
+ return *TheRules;
}
// TODO: make AccessRules::find work
Adaptation::AccessRule *
Adaptation::FindRule(const AccessRule::Id &id)
{
typedef AccessRules::iterator ARI;
for (ARI i = AllRules().begin(); i != AllRules().end(); ++i) {
if ((*i)->id == id)
return *i;
}
return NULL;
}
Adaptation::AccessRule *
Adaptation::FindRuleByGroupId(const String &groupId)
{
typedef AccessRules::iterator ARI;
for (ARI i = AllRules().begin(); i != AllRules().end(); ++i) {
=== modified file 'src/adaptation/Service.cc'
--- src/adaptation/Service.cc 2014-02-04 19:47:14 +0000
+++ src/adaptation/Service.cc 2014-02-20 01:28:25 +0000
@@ -37,42 +37,42 @@ Adaptation::Service::wants(const Service
// sending a message to a broken service is likely to cause errors
if (cfg().bypass && broken())
return false;
if (up()) {
// Sending a message to a service that does not want it is useless.
// note that we cannot check wantsUrl for service that is not "up"
// note that even essential services are skipped on unwanted URLs!
return wantsUrl(filter.request->urlpath);
}
// The service is down and is either not bypassable or not probed due
// to the bypass && broken() test above. Thus, we want to use it!
return true;
}
Adaptation::Services &
Adaptation::AllServices()
{
- static Services TheServices;
- return TheServices;
+ static Services *TheServices = new Services;
+ return *TheServices;
}
Adaptation::ServicePointer
Adaptation::FindService(const Service::Id& key)
{
typedef Services::iterator SI;
for (SI i = AllServices().begin(); i != AllServices().end(); ++i) {
if ((*i)->cfg().key == key)
return *i;
}
return NULL;
}
void Adaptation::DetachServices()
{
while (!AllServices().empty()) {
AllServices().back()->detach();
AllServices().pop_back();
}
}
=== modified file 'src/adaptation/ServiceGroups.cc'
--- src/adaptation/ServiceGroups.cc 2014-02-02 09:42:23 +0000
+++ src/adaptation/ServiceGroups.cc 2014-02-20 01:26:45 +0000
@@ -298,35 +298,35 @@ Adaptation::ServicePlan::next(const Serv
if (!atEof && !group->findLink(filter, ++pos))
atEof = true;
return current();
}
std::ostream &
Adaptation::ServicePlan::print(std::ostream &os) const
{
if (!group)
return os << "[nil]";
return os << group->id << '[' << pos << ".." << group->services.size() <<
(atEof ? ".]" : "]");
}
/* globals */
Adaptation::Groups &
Adaptation::AllGroups()
{
- static Groups TheGroups;
- return TheGroups;
+ static Groups *TheGroups = new Groups;
+ return *TheGroups;
}
Adaptation::ServiceGroupPointer
Adaptation::FindGroup(const ServiceGroup::Id &id)
{
typedef Groups::iterator GI;
for (GI i = AllGroups().begin(); i != AllGroups().end(); ++i) {
if ((*i)->id == id)
return *i;
}
return NULL;
}
