diff --git a/env/env_posix.cc b/env/env_posix.cc index 77f28e1f5..400b8ac6e 100644 --- a/env/env_posix.cc +++ b/env/env_posix.cc @@ -213,13 +213,14 @@ class PosixEnv : public CompositeEnv { const char* Name() const override { return kClassName(); } const char* NickName() const override { return kDefaultName(); } - ~PosixEnv() override { - if (this == Env::Default()) { - for (const auto tid : threads_to_join_) { + struct JoinThreadsOnExit { + explicit JoinThreadsOnExit(PosixEnv& _deflt) : deflt(_deflt) {} + ~JoinThreadsOnExit() { + for (const auto tid : deflt.threads_to_join_) { pthread_join(tid, nullptr); } for (int pool_id = 0; pool_id < Env::Priority::TOTAL; ++pool_id) { - thread_pools_[pool_id].JoinAllThreads(); + deflt.thread_pools_[pool_id].JoinAllThreads(); } // Do not delete the thread_status_updater_ in order to avoid the // free after use when Env::Default() is destructed while some other @@ -227,7 +228,8 @@ class PosixEnv : public CompositeEnv { // PosixEnv instances use the same thread_status_updater_, so never // explicitly delete it. } - } + PosixEnv& deflt; + }; void SetFD_CLOEXEC(int fd, const EnvOptions* options) { if ((options == nullptr || options->set_fd_cloexec) && fd > 0) { @@ -501,9 +503,11 @@ Env* Env::Default() { ThreadLocalPtr::InitSingletons(); CompressionContextCache::InitSingleton(); INIT_SYNC_POINT_SINGLETONS(); - // ~PosixEnv must be called on exit - //**TODO: Can we make this a STATIC_AVOID_DESTRUCTION? - static PosixEnv default_env; + // Avoid problems with accessing most members of Env::Default() during + // static destruction. + STATIC_AVOID_DESTRUCTION(PosixEnv, default_env); + // This destructor must be called on exit + static PosixEnv::JoinThreadsOnExit thread_joiner(default_env); return &default_env; } diff --git a/env/env_test.cc b/env/env_test.cc index 926e2f86f..eedec9bea 100644 --- a/env/env_test.cc +++ b/env/env_test.cc @@ -3551,6 +3551,23 @@ TEST_F(TestAsyncRead, ReadAsync) { } } } + +struct StaticDestructionTester { + bool activated = false; + ~StaticDestructionTester() { + if (activated && !kMustFreeHeapAllocations) { + // Make sure we can still call some things on default Env. + std::string hostname; + Env::Default()->GetHostNameString(&hostname); + } + } +} static_destruction_tester; + +TEST(EnvTestMisc, StaticDestruction) { + // Check for any crashes during static destruction. + static_destruction_tester.activated = true; +} + } // namespace ROCKSDB_NAMESPACE int main(int argc, char** argv) { diff --git a/unreleased_history/bug_fixes/destroy_posix_env.md b/unreleased_history/bug_fixes/destroy_posix_env.md new file mode 100644 index 000000000..396640d6b --- /dev/null +++ b/unreleased_history/bug_fixes/destroy_posix_env.md @@ -0,0 +1 @@ +Reduced cases of illegally using Env::Default() during static destruction by never destroying the internal PosixEnv itself (except for builds checking for memory leaks). (#11538)