Preparing for shutdown after 186724 requests
Waiting 1 seconds for active connections to finish
Closing HTTP(S) port [::]:3128
FATAL: assertion failed: Transients.cc:192:
"oldE->mem_obj && oldE->mem_obj->xitTable.index == index"
This change removes two steps at the end of the manual shutdown sequence
(FreeAllFs() and Store::FreeMemory()) and associated deadly Store hacks,
adjusting a few Store checks as needed. Also, StoreFileSystem hierarchy
declarations were slightly updated when half of its virtual methods were
removed. Key changes are detailed below.
### Do not disable Store [by dropping Store::Root()] during shutdown
Disabling Store (i.e. calling Store::FreeMemory()) resulted in "do not
use Root() during shutdown" assertion-avoiding hacks that bypassed code
maintaining key invariants. Those hacks were probably accommodating
Store-using destruction sequences triggered by SquidShutdown() or later
code, _but_ the hacks also ran during the graceful shutdown period!
Violating those invariants in MemObject destructor led to the quoted
assertions when CollapsedForwarding::HandleNotification() ran during
graceful shutdown.
We could add more state to narrow down hacks scope, or we could disable
IPC notifications that triggered the known deadly shutdown sequence, but
* We should not disable assertions during shutdown because many are
about essential (e.g., preventing data corruption) invariants and
because it is not practical to disable all assertions that should be
disabled (and keep disabling the new ones correctly). The idea leads
to unsafe code that is also more difficult to write and maintain.
* We should not disable kid-to-kid notifications because some are
required for finishing transactions (that can still be successfully
finished) during graceful shutdown. Increasing successful completion
chances is the whole idea behind that graceful shutdown feature!
Instead, Store should provide (essential) services during shutdown.
TODO: Update those unit test cases that use Store::FreeMemory() but
should not. That requires careful analysis because the next test cases
may rely on the previous case removing its Store object/statistics/etc.
The same dedicated project might (help) address commit
27685ef TODO
about unwanted Store unit test order dependencies.
### Do not disable StoreFileSystem service during shutdown
Similar to the Store service, manually killing FS service is a bad idea.
The underlying code was also essentially unused and buggy:
* Nobody called RegisterAllFsWithCacheManager(). A caller would assert.
* Nobody called Fs::Clean(). A call could crash Squid because deleted
objects could still be in use by others (they are not refcounted).
* SetupAllFs() was called but did essentially nothing.
* FreeAllFs() was called but had only harmful effects: It leaked memory
and removed usable FS registrations, increasing shutdown dangers.
See also: Commit
3a85184 documents why we should avoid explicit "delete
essentialService" calls (and why some crashes may still occur if we do).
checkUrlChecksum();
#endif
- if (!shutting_down) { // Store::Root() is FATALly missing during shutdown
- assert(xitTable.index < 0);
- assert(memCache.index < 0);
- assert(swapout.sio == nullptr);
- }
+ assert(xitTable.index < 0);
+ assert(memCache.index < 0);
+ assert(swapout.sio == nullptr);
data_hdr.freeContent();
}
return false;
}
- // Store::Root() in the next check below is FATALly missing during shutdown
if (shutting_down) {
- debugs(20, 5, "yield to shutdown: " << e);
+ debugs(20, 5, "avoid heavy optional work during shutdown: " << e);
return false;
}
std::vector<StoreFileSystem*> *StoreFileSystem::_FileSystems = nullptr;
-void
-StoreFileSystem::RegisterAllFsWithCacheManager(void)
-{
- for (iterator i = GetFileSystems().begin(); i != GetFileSystems().end(); ++i)
- (*i)->registerWithCacheManager();
-}
-
-void
-StoreFileSystem::SetupAllFs()
-{
- for (iterator i = GetFileSystems().begin(); i != GetFileSystems().end(); ++i)
- /* Call the FS to set up capabilities and initialize the FS driver */
- (*i)->setup();
-}
-
void
StoreFileSystem::FsAdd(StoreFileSystem &instance)
{
return *_FileSystems;
}
-/*
- * called when a graceful shutdown is to occur
- * of each fs module.
- */
-void
-StoreFileSystem::FreeAllFs()
-{
- while (!GetFileSystems().empty()) {
- StoreFileSystem *fs = GetFileSystems().back();
- GetFileSystems().pop_back();
- fs->done();
- }
-}
-
StoreFileSystem *
StoreFileSystem::FindByType(const char *type)
{
return nullptr;
}
-/* no filesystem is required to export statistics */
-void
-StoreFileSystem::registerWithCacheManager(void)
-{}
-
#ifndef SQUID_STOREFILESYSTEM_H
#define SQUID_STOREFILESYSTEM_H
+#include "base/TypeTraits.h"
#include "store/forward.h"
#include <vector>
* The core API for storage modules this class provides all the hooks
* squid uses to interact with a filesystem IO module.
*/
-class StoreFileSystem
+class StoreFileSystem: public Interface
{
public:
- static void SetupAllFs();
static void FsAdd(StoreFileSystem &);
- static void FreeAllFs();
static StoreFileSystem *FindByType(const char *type);
static std::vector<StoreFileSystem*> const &FileSystems();
typedef std::vector<StoreFileSystem*>::iterator iterator;
typedef std::vector<StoreFileSystem*>::const_iterator const_iterator;
- StoreFileSystem() : initialised(false) {}
- virtual ~StoreFileSystem() {}
+ StoreFileSystem() = default;
+ StoreFileSystem(StoreFileSystem &&) = delete; // no copying/moving of any kind
virtual char const *type () const = 0;
virtual SwapDir *createSwapDir() = 0;
- virtual void done() = 0;
- virtual void setup() = 0;
- // Not implemented
- StoreFileSystem(StoreFileSystem const &);
- StoreFileSystem &operator=(StoreFileSystem const&);
-
-protected:
- bool initialised;
- virtual void registerWithCacheManager(void);
private:
static std::vector<StoreFileSystem*> &GetFileSystems();
static std::vector<StoreFileSystem*> *_FileSystems;
- static void RegisterAllFsWithCacheManager(void);
};
// TODO: Kill this typedef!
}
-void Fs::Clean()
-{
-#if HAVE_FS_UFS
- delete UfsInstance;
-#endif
-
-#if HAVE_FS_AUFS
- delete AufsInstance;
-#endif
-
-#if HAVE_FS_DISKD
- delete DiskdInstance;
-#endif
-
-#if HAVE_FS_ROCK
- delete RockInstance;
-#endif
-
-}
-
{
void Init();
-void Clean();
} // namespace Fs
return new SwapDir();
}
-void
-Rock::StoreFileSystem::done()
-{
-}
-
-void
-Rock::StoreFileSystem::registerWithCacheManager()
-{
- assert(false); // XXX: implement
-}
-
-void
-Rock::StoreFileSystem::setup()
-{
- debugs(92,2, "Will use Rock FS");
-}
-
-void
-Rock::StoreFileSystem::Stats(StoreEntry *)
-{
- assert(false); // XXX: implement
-}
-
StoreFileSystem();
virtual ~StoreFileSystem();
- virtual char const *type() const;
- virtual SwapDir *createSwapDir();
- virtual void done();
- virtual void registerWithCacheManager();
- virtual void setup();
+ /* StoreFileSystem API */
+ virtual char const *type() const override;
+ virtual SwapDir *createSwapDir() override;
private:
//static Stats Stats_;
StoreFSufs(char const *DefaultModuleType, char const *label);
virtual ~StoreFSufs() {}
- virtual char const *type() const;
- virtual SwapDir *createSwapDir();
- virtual void done();
- virtual void setup();
- /** Not implemented */
- StoreFSufs (StoreFSufs const &);
- StoreFSufs &operator=(StoreFSufs const &);
+ /* StoreFileSystem API */
+ virtual const char *type() const override;
+ virtual SwapDir *createSwapDir() override;
protected:
DiskIOModule *IO;
return result;
}
-template <class C>
-void
-StoreFSufs<C>::done()
-{
- initialised = false;
-}
-
-template <class C>
-void
-StoreFSufs<C>::setup()
-{
- assert(!initialised);
- initialised = true;
-}
-
} /* namespace Ufs */
} /* namespace Fs */
storeFsInit(); /* required for config parsing */
- /* TODO: call the FS::Clean() in shutdown to do Fs cleanups */
Fs::Init();
/* May not be needed for parsing, have not audited for such */
DiskIOModule::SetupAllModules();
- /* Shouldn't be needed for config parsing, but have not audited for such */
- StoreFileSystem::SetupAllFs();
-
/* we may want the parsing process to set this up in the future */
Store::Init();
Acl::Init();
storeLogClose();
accessLogClose();
Store::Root().sync(); /* Flush log close */
- StoreFileSystem::FreeAllFs();
DiskIOModule::FreeAllModules();
- Store::FreeMemory();
fdDumpOpen();
{
debugs(20, 3, mem_obj << " in " << *this);
- // Store::Root() is FATALly missing during shutdown
- if (hasTransients() && !shutting_down)
+ if (hasTransients())
Store::Root().transientsDisconnect(*this);
- if (hasMemStore() && !shutting_down)
+ if (hasMemStore())
Store::Root().memoryDisconnect(*this);
if (auto memObj = mem_obj) {
StoreEntry *e = static_cast<StoreEntry *>(static_cast<hash_link *>(data));
assert(e != nullptr);
- // Store::Root() is FATALly missing during shutdown
- if (e->hasDisk() && !shutting_down)
+ if (e->hasDisk())
e->disk().disconnect(*e);
e->destroyMemObject();
void
storeGetMemSpace(int size)
{
- if (!shutting_down) // Store::Root() is FATALly missing during shutdown
- Store::Root().freeMemorySpace(size);
+ Store::Root().freeMemorySpace(size);
}
/* thunk through to Store::Root().maintain(). Note that this would be better still
if (!checkCachable())
return 0;
+ if (shutting_down)
+ return 0; // avoid heavy optional work during shutdown
+
if (mem_obj == nullptr)
return 0;
}
debugs(20, 7, "done with writing " << *this);
- if (!shutting_down) // Store::Root() is FATALly missing during shutdown
- Store::Root().noteStoppedSharedWriting(*this);
+ Store::Root().noteStoppedSharedWriting(*this);
}
void
return false;
}
- if (!shutting_down && Store::Root().transientReaders(*entry)) {
+ if (Store::Root().transientReaders(*entry)) {
debugs(90, 3, "quick-abort? NO still have one or more transient readers");
return false;
}
return false;
}
+ if (shutting_down) {
+ debugs(90, 3, "quick-abort? YES avoid heavy optional work during shutdown");
+ return true;
+ }
+
MemObject * const mem = entry->mem_obj;
assert(mem);
debugs(90, 3, "mem=" << mem);
void
StoreEntry::swapOut()
{
- // Store::Root() in many swapout checks is FATALly missing during shutdown
- if (shutting_down)
- return;
-
if (!mem_obj)
return;
return false;
}
+ if (shutting_down) {
+ debugs(20, 3, "avoid heavy optional work during shutdown");
+ swapOutDecision(MemObject::SwapOut::swImpossible);
+ return false;
+ }
+
// if there is a usable disk entry already, do not start over
if (hasDisk() || Store::Root().hasReadableDiskEntry(*this)) {
debugs(20, 3, "already did"); // we or somebody else created that entry
if (inited)
return;
- StoreFileSystem::SetupAllFs();
-
Config.Store.avgObjectSize = 1024;
Config.Store.objectsPerBucket = 20;
Config.Store.maxObjectSize = 2048;
projects / thirdparty / squid.git / commitdiff
