From 1f50e07b88dc02c1413a87f10128d29c12937e02 Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Wed, 28 Feb 2024 19:27:15 +0000 Subject: [PATCH] Fix Controller.cc TheRoot assertion during shutdown (#1707) FATAL: assertion failed: Controller.cc:933: "TheRoot" Store is an essential service, used by a lot of Squid code. As was already established in 2022 commit 23b7963, this service should be available during shutdown. That commit correctly removed explicit Store service termination, but missed the fact that the reference-counting TheRoot pointer (that provides access to the Store Controller singleton) gets _automatically_ destroyed during C++ cleanup. This change removes TheRoot reference counting, making that Controller singleton immortal. Squid asserted when exiting with active entries in the shared memory cache because TheRoot destruction leads to Controller destruction, Controller destructor cleans up cache index, and that cleanup code may result in calls to Store::Root() that dereferences destroyed TheRoot. These assertions were seen when Squid was shutdown cleanly (e.g., using SIGTERM) and when a kid process was exiting due to a fatal() error. Making Store::Controller singleton immortal means that the class destructor is never called. Fortunately, the destructor did nothing particularly useful; Store flushing is performed by Controller::sync() which is explicitly called during early stages of clean shutdown. The now-unused destructor code was removed: Implementing this destructor in a meaningful way (while avoiding accessing a being-destructed global Store!) requires heroic efforts (which would be wasted since the destructor is never actually called). Also made Store Controller singleton available on the first use, complying with Store's "essential, widely used service" status and removing the need for an explicit Store::Init(void) call. Also removed unit tests that required support for dynamic replacement of Store Controller singleton. The minuscule value of the removed tests did not justify the costs of supporting a replaceable Store Controller. Also removed Store::FreeMemory(). Its implementation contradicted Store status as an essential service. The function was only used by unit tests, and its removal addresses the corresponding commit 23b7963 TODO. --- src/main.cc | 1 - src/store/Controller.cc | 32 ++------- src/store/Controller.h | 9 +-- src/tests/stub_libstore.cc | 7 +- src/tests/testPackableStream.cc | 4 -- src/tests/testRock.cc | 4 -- src/tests/testStore.cc | 110 ------------------------------- src/tests/testStore.h | 53 --------------- src/tests/testStoreController.cc | 7 -- src/tests/testStoreHashIndex.cc | 7 -- src/tests/testUfs.cc | 11 ---- 11 files changed, 9 insertions(+), 236 deletions(-) diff --git a/src/main.cc b/src/main.cc index 040c7f1e7b..5185977503 100644 --- a/src/main.cc +++ b/src/main.cc @@ -1585,7 +1585,6 @@ SquidMain(int argc, char **argv) DiskIOModule::SetupAllModules(); /* we may want the parsing process to set this up in the future */ - Store::Init(); Acl::Init(); Auth::Init(); /* required for config parsing. NOP if !USE_AUTH */ Ip::ProbeTransport(); // determine IPv4 or IPv6 capabilities before parsing. diff --git a/src/store/Controller.cc b/src/store/Controller.cc index 5f5bad1f30..b9dac9bad6 100644 --- a/src/store/Controller.cc +++ b/src/store/Controller.cc @@ -41,17 +41,11 @@ Store::Controller::Controller() : assert(!store_table); } +/// this destructor is never called because Controller singleton is immortal Store::Controller::~Controller() { - delete sharedMemStore; - delete transients; - delete disks; - - if (store_table) { - hashFreeItems(store_table, destroyStoreEntry); - hashFreeMemory(store_table); - store_table = nullptr; - } + // assert at runtime because we cannot `= delete` an overridden destructor + assert(!"Controller is never destroyed"); } void @@ -927,26 +921,10 @@ Store::Controller::checkTransients(const StoreEntry &e) const assert(!transients || e.hasTransients()); } -namespace Store { -static RefCount TheRoot; -} - Store::Controller& Store::Root() { - assert(TheRoot); - return *TheRoot; -} - -void -Store::Init(Controller *root) -{ - TheRoot = root ? root : new Controller; -} - -void -Store::FreeMemory() -{ - TheRoot = nullptr; + static const auto root = new Controller(); + return *root; } diff --git a/src/store/Controller.h b/src/store/Controller.h index 7930dd6f66..aec96d06cd 100644 --- a/src/store/Controller.h +++ b/src/store/Controller.h @@ -23,7 +23,6 @@ class Controller: public Storage { public: Controller(); - ~Controller() override; /* Storage API */ void create() override; @@ -134,6 +133,8 @@ public: static int store_dirs_rebuilding; private: + ~Controller() override; + bool memoryCacheHasSpaceFor(const int pagesRequired) const; void referenceBusy(StoreEntry &e); @@ -165,12 +166,6 @@ private: /// safely access controller singleton extern Controller &Root(); -/// initialize the storage module; a custom root is used by unit tests only -extern void Init(Controller *root = nullptr); - -/// undo Init() -extern void FreeMemory(); - } // namespace Store #endif /* SQUID_SRC_STORE_CONTROLLER_H */ diff --git a/src/tests/stub_libstore.cc b/src/tests/stub_libstore.cc index 3d6dfaacca..d18de336e8 100644 --- a/src/tests/stub_libstore.cc +++ b/src/tests/stub_libstore.cc @@ -15,7 +15,7 @@ namespace Store { Controller::Controller() {STUB_NOP} -Controller::~Controller() {STUB_NOP} +Controller::~Controller() STUB void Controller::create() STUB void Controller::init() STUB uint64_t Controller::maxSize() const STUB_RETVAL(0) @@ -55,10 +55,7 @@ void Controller::memoryDisconnect(StoreEntry &) STUB StoreSearch *Controller::search() STUB_RETVAL(nullptr) bool Controller::SmpAware() STUB_RETVAL(false) int Controller::store_dirs_rebuilding = 0; -Controller nil; -Controller &Root() STUB_RETVAL(Store::nil) -void Init(Controller *) STUB -void FreeMemory() STUB +Controller &Root() STUB_RETREF(Controller) } #include "store/Disk.h" diff --git a/src/tests/testPackableStream.cc b/src/tests/testPackableStream.cc index d89adb98be..7702a0a661 100644 --- a/src/tests/testPackableStream.cc +++ b/src/tests/testPackableStream.cc @@ -33,9 +33,6 @@ CPPUNIT_TEST_SUITE_REGISTRATION( TestPackableStream ); void TestPackableStream::testGetStream() { - /* Setup a store root so we can create a StoreEntry */ - Store::Init(); - CapturingStoreEntry * anEntry = new CapturingStoreEntry(); { anEntry->lock("test"); @@ -59,7 +56,6 @@ TestPackableStream::testGetStream() CPPUNIT_ASSERT_EQUAL(String("12345677.7 some text !."), anEntry->_appended_text); } delete anEntry; // does the unlock() - Store::FreeMemory(); } // This test uses main() from ./testStore.cc. diff --git a/src/tests/testRock.cc b/src/tests/testRock.cc index 0eace9fd61..bb89e8c529 100644 --- a/src/tests/testRock.cc +++ b/src/tests/testRock.cc @@ -85,8 +85,6 @@ TestRock::setUp() if (0 > system ("rm -rf " TESTDIR)) throw std::runtime_error("Failed to clean test work directory"); - Store::Init(); - store = new Rock::SwapDir(); addSwapDir(store); @@ -116,8 +114,6 @@ TestRock::tearDown() { CPPUNIT_NS::TestFixture::tearDown(); - Store::FreeMemory(); - store = nullptr; free_cachedir(&Config.cacheSwap); diff --git a/src/tests/testStore.cc b/src/tests/testStore.cc index 54e1c96ccd..419326949e 100644 --- a/src/tests/testStore.cc +++ b/src/tests/testStore.cc @@ -16,116 +16,6 @@ CPPUNIT_TEST_SUITE_REGISTRATION( TestStore ); -int -StoreControllerStub::callback() -{ - return 1; -} - -StoreEntry* -StoreControllerStub::get(const cache_key*) -{ - return nullptr; -} - -void -StoreControllerStub::get(String, void (*)(StoreEntry*, void*), void*) -{} - -void -StoreControllerStub::init() -{} - -uint64_t -StoreControllerStub::maxSize() const -{ - return 3; -} - -uint64_t -StoreControllerStub::minSize() const -{ - return 1; -} - -uint64_t -StoreControllerStub::currentSize() const -{ - return 2; -} - -uint64_t -StoreControllerStub::currentCount() const -{ - return 2; -} - -int64_t -StoreControllerStub::maxObjectSize() const -{ - return 1; -} - -void -StoreControllerStub::getStats(StoreInfoStats &) const -{ -} - -void -StoreControllerStub::stat(StoreEntry &) const -{ - const_cast(this)->statsCalled = true; -} - -StoreSearch * -StoreControllerStub::search() -{ - return nullptr; -} - -void -TestStore::testSetRoot() -{ - Store::Controller *aStore(new StoreControllerStub); - Store::Init(aStore); - - CPPUNIT_ASSERT_EQUAL(&Store::Root(), aStore); - Store::FreeMemory(); -} - -void -TestStore::testUnsetRoot() -{ - Store::Controller *aStore(new StoreControllerStub); - Store::Controller *aStore2(new StoreControllerStub); - Store::Init(aStore); - Store::FreeMemory(); - Store::Init(aStore2); - CPPUNIT_ASSERT_EQUAL(&Store::Root(),aStore2); - Store::FreeMemory(); -} - -void -TestStore::testStats() -{ - StoreControllerStub *aStore(new StoreControllerStub); - Store::Init(aStore); - CPPUNIT_ASSERT_EQUAL(false, aStore->statsCalled); - StoreEntry entry; - Store::Stats(&entry); - CPPUNIT_ASSERT_EQUAL(true, aStore->statsCalled); - Store::FreeMemory(); -} - -void -TestStore::testMaxSize() -{ - Store::Controller *aStore(new StoreControllerStub); - Store::Init(aStore); - CPPUNIT_ASSERT_EQUAL(static_cast(3), aStore->maxSize()); - Store::FreeMemory(); -} - namespace Store { /// check rawType that may be ignored diff --git a/src/tests/testStore.h b/src/tests/testStore.h index accf6d9b74..4c8dd3c9cf 100644 --- a/src/tests/testStore.h +++ b/src/tests/testStore.h @@ -9,9 +9,6 @@ #ifndef SQUID_SRC_TESTS_TESTSTORE_H #define SQUID_SRC_TESTS_TESTSTORE_H -#include "Store.h" -#include "store/Controlled.h" - #include "compat/cppunit.h" /* @@ -21,64 +18,14 @@ class TestStore: public CPPUNIT_NS::TestFixture { CPPUNIT_TEST_SUITE( TestStore ); - CPPUNIT_TEST( testSetRoot ); - CPPUNIT_TEST( testUnsetRoot ); - CPPUNIT_TEST( testStats ); - CPPUNIT_TEST( testMaxSize ); CPPUNIT_TEST( testSwapMetaTypeClassification ); CPPUNIT_TEST_SUITE_END(); public: protected: - void testSetRoot(); - void testUnsetRoot(); - void testStats(); - void testMaxSize(); void testSwapMetaTypeClassification(); }; -/// allows testing of methods without having all the other components live -class StoreControllerStub : public Store::Controller -{ - -public: - StoreControllerStub() : statsCalled (false) {} - - bool statsCalled; - - int callback() override; - - virtual StoreEntry* get(const cache_key*); - - virtual void get(String, void (*)(StoreEntry*, void*), void*); - - void init() override; - - void maintain() override {}; - - uint64_t maxSize() const override; - - uint64_t minSize() const override; - - uint64_t currentSize() const override; - - uint64_t currentCount() const override; - - int64_t maxObjectSize() const override; - - void getStats(StoreInfoStats &) const override; - - void stat(StoreEntry &) const override; /* output stats to the provided store entry */ - - virtual void reference(StoreEntry &) {} /* Reference this object */ - - virtual bool dereference(StoreEntry &) { return true; } - - virtual StoreSearch *search(); -}; - -typedef RefCount StoreControllerStubPointer; - #endif /* SQUID_SRC_TESTS_TESTSTORE_H */ diff --git a/src/tests/testStoreController.cc b/src/tests/testStoreController.cc index 06e8f8fbb0..205dc59b8c 100644 --- a/src/tests/testStoreController.cc +++ b/src/tests/testStoreController.cc @@ -46,7 +46,6 @@ addSwapDir(TestSwapDirPointer aStore) void TestStoreController::testStats() { - Store::Init(); StoreEntry *logEntry = new StoreEntry; logEntry->createMemObject("dummy_storeId", nullptr, HttpRequestMethod()); logEntry->store_status = STORE_PENDING; @@ -60,7 +59,6 @@ TestStoreController::testStats() free_cachedir(&Config.cacheSwap); CPPUNIT_ASSERT_EQUAL(true, aStore->statsCalled); CPPUNIT_ASSERT_EQUAL(true, aStore2->statsCalled); - Store::FreeMemory(); } static void @@ -91,14 +89,12 @@ TestStoreController::testMaxSize() StoreEntry *logEntry = new StoreEntry; logEntry->createMemObject("dummy_storeId", nullptr, HttpRequestMethod()); logEntry->store_status = STORE_PENDING; - Store::Init(); TestSwapDirPointer aStore (new TestSwapDir); TestSwapDirPointer aStore2 (new TestSwapDir); addSwapDir(aStore); addSwapDir(aStore2); CPPUNIT_ASSERT_EQUAL(static_cast(6), Store::Root().maxSize()); free_cachedir(&Config.cacheSwap); - Store::FreeMemory(); } static StoreEntry * @@ -147,7 +143,6 @@ void TestStoreController::testSearch() { commonInit(); - Store::Init(); TestSwapDirPointer aStore (new TestSwapDir); TestSwapDirPointer aStore2 (new TestSwapDir); addSwapDir(aStore); @@ -195,8 +190,6 @@ TestStoreController::testSearch() CPPUNIT_ASSERT_EQUAL(true, search->isDone()); CPPUNIT_ASSERT_EQUAL(static_cast(nullptr), search->currentItem()); //CPPUNIT_ASSERT_EQUAL(false, search->next()); - - Store::FreeMemory(); } // This test uses main() from ./testStore.cc. diff --git a/src/tests/testStoreHashIndex.cc b/src/tests/testStoreHashIndex.cc index 72813338de..232664e8c7 100644 --- a/src/tests/testStoreHashIndex.cc +++ b/src/tests/testStoreHashIndex.cc @@ -49,7 +49,6 @@ TestStoreHashIndex::testStats() StoreEntry *logEntry = new StoreEntry; logEntry->createMemObject("dummy_storeId", nullptr, HttpRequestMethod()); logEntry->store_status = STORE_PENDING; - Store::Init(); TestSwapDirPointer aStore (new TestSwapDir); TestSwapDirPointer aStore2 (new TestSwapDir); addSwapDir(aStore); @@ -60,7 +59,6 @@ TestStoreHashIndex::testStats() free_cachedir(&Config.cacheSwap); CPPUNIT_ASSERT_EQUAL(true, aStore->statsCalled); CPPUNIT_ASSERT_EQUAL(true, aStore2->statsCalled); - Store::FreeMemory(); } void @@ -69,14 +67,12 @@ TestStoreHashIndex::testMaxSize() StoreEntry *logEntry = new StoreEntry; logEntry->createMemObject("dummy_storeId", nullptr, HttpRequestMethod()); logEntry->store_status = STORE_PENDING; - Store::Init(); TestSwapDirPointer aStore (new TestSwapDir); TestSwapDirPointer aStore2 (new TestSwapDir); addSwapDir(aStore); addSwapDir(aStore2); CPPUNIT_ASSERT_EQUAL(static_cast(6), Store::Root().maxSize()); free_cachedir(&Config.cacheSwap); - Store::FreeMemory(); } static StoreEntry * @@ -147,7 +143,6 @@ void TestStoreHashIndex::testSearch() { commonInit(); - Store::Init(); TestSwapDirPointer aStore (new TestSwapDir); TestSwapDirPointer aStore2 (new TestSwapDir); addSwapDir(aStore); @@ -195,8 +190,6 @@ TestStoreHashIndex::testSearch() CPPUNIT_ASSERT_EQUAL(true, search->isDone()); CPPUNIT_ASSERT_EQUAL(static_cast(nullptr), search->currentItem()); //CPPUNIT_ASSERT_EQUAL(false, search->next()); - - Store::FreeMemory(); } // This test uses main() from ./testStore.cc. diff --git a/src/tests/testUfs.cc b/src/tests/testUfs.cc index cfdad7498f..5daec5e5b9 100644 --- a/src/tests/testUfs.cc +++ b/src/tests/testUfs.cc @@ -110,8 +110,6 @@ TestUfs::testUfsSearch() if (0 > system ("rm -rf " TESTDIR)) throw std::runtime_error("Failed to clean test work directory"); - Store::Init(); - MySwapDirPointer aStore (new Fs::Ufs::UFSSwapDir("ufs", "Blocking")); aStore->IO = new Fs::Ufs::UFSStrategy(DiskIOModule::Find("Blocking")->createStrategy()); @@ -213,8 +211,6 @@ TestUfs::testUfsSearch() CPPUNIT_ASSERT_EQUAL(true, search->isDone()); CPPUNIT_ASSERT_EQUAL(static_cast(nullptr), search->currentItem()); - Store::FreeMemory(); - free_cachedir(&Config.cacheSwap); // TODO: here we should test a dirty rebuild @@ -236,12 +232,6 @@ TestUfs::testUfsDefaultEngine() if (0 > system ("rm -rf " TESTDIR)) throw std::runtime_error("Failed to clean test work directory"); - // This assertion may fail if previous test cases fail. - // Apparently, CPPUNIT_ASSERT* failure may prevent destructors of local - // objects such as "StorePointer aRoot" from being called. - CPPUNIT_ASSERT(!store_table); // or StoreHashIndex ctor will abort below - - Store::Init(); MySwapDirPointer aStore (new Fs::Ufs::UFSSwapDir("ufs", "Blocking")); addSwapDir(aStore); commonInit(); @@ -257,7 +247,6 @@ TestUfs::testUfsDefaultEngine() safe_free(config_line); CPPUNIT_ASSERT(aStore->IO->io != nullptr); - Store::FreeMemory(); free_cachedir(&Config.cacheSwap); safe_free(Config.replPolicy->type); delete Config.replPolicy; -- 2.47.2