From ba114dc8501256b4831810526984d1aff25147de Mon Sep 17 00:00:00 2001 From: Igor Solodovnikov Date: Fri, 20 Nov 2015 02:13:16 +0200 Subject: [PATCH] SERVER-21311 fixing crash on shutdown in RocksRecoveryUnit::dbReleaseSnapshot Problem description: The crash was caused by dangling pointer to RocksRecoveryUnit stored in the instance of RocksSnapshotManager::SnapshotHolder class. Crash scenario: - execute administrative 'makeSnapshot' command. This will create instance of RocksSnapshotManager::SnapshotHolder which is stored in RocksSnapshotManager's snapshots map. - SnapshotHolder has raw pointer to RocksRecoveryUnit instance which is destroyed at the end of 'makeSnapshot' command handler - destructor of SnapshotHolder is called on shutdown when snapshot maps are cleared in RocksSnapshotManager::dropAllSnapshots - destructor of SnapshotHolder tries to release snapshot using stored pointer to RocksRecoveryUnit instance which was destroyed earlier Fix description: Instead of storing RocksRecoveryUnit pointer in SnapshotHolder we will store pointer to rocksdb::DB instance. Thus we will be able to call rocks::DB::ReleaseSnapshot() from SnapshotHolder's destructor and SnapshotHolder will be independent of RocksRecoveryUnit lifetime. --- src/rocks_recovery_unit.h | 2 ++ src/rocks_snapshot_manager.cpp | 9 +++++---- src/rocks_snapshot_manager.h | 2 +- 3 files changed, 8 insertions(+), 5 deletions(-) diff --git a/src/rocks_recovery_unit.h b/src/rocks_recovery_unit.h index c026044a..4d058d7e 100644 --- a/src/rocks_recovery_unit.h +++ b/src/rocks_recovery_unit.h @@ -160,6 +160,8 @@ namespace mongo { void setCommittedSnapshot(const rocksdb::Snapshot* committedSnapshot); + rocksdb::DB* getDB() const { return _db; } + private: void _releaseSnapshot(); diff --git a/src/rocks_snapshot_manager.cpp b/src/rocks_snapshot_manager.cpp index a2976a73..fac7f249 100644 --- a/src/rocks_snapshot_manager.cpp +++ b/src/rocks_snapshot_manager.cpp @@ -102,14 +102,15 @@ namespace mongo { RocksSnapshotManager::SnapshotHolder::SnapshotHolder(OperationContext* opCtx, uint64_t name_) { name = name_; - ru = RocksRecoveryUnit::getRocksRecoveryUnit(opCtx); - snapshot = ru->getPreparedSnapshot(); + auto rru = RocksRecoveryUnit::getRocksRecoveryUnit(opCtx); + snapshot = rru->getPreparedSnapshot(); + db = rru->getDB(); } RocksSnapshotManager::SnapshotHolder::~SnapshotHolder() { if (snapshot != nullptr) { - invariant(ru != nullptr); - ru->dbReleaseSnapshot(snapshot); + invariant(db != nullptr); + db->ReleaseSnapshot(snapshot); } } diff --git a/src/rocks_snapshot_manager.h b/src/rocks_snapshot_manager.h index 61813aa3..11e93847 100644 --- a/src/rocks_snapshot_manager.h +++ b/src/rocks_snapshot_manager.h @@ -48,7 +48,7 @@ class RocksSnapshotManager final : public SnapshotManager { struct SnapshotHolder { uint64_t name; const rocksdb::Snapshot* snapshot; - RocksRecoveryUnit* ru; + rocksdb::DB* db; SnapshotHolder(OperationContext* opCtx, uint64_t name_); ~SnapshotHolder(); };