Skip to content

Commit

Permalink
Free BerkeleyEnvironment instances when not in use
Browse files Browse the repository at this point in the history 
Instead of adding BerkeleyEnvironment objects permanently to the g_dbenvs map,
use reference counted shared pointers and remove map entries when the last
BerkeleyEnvironment reference goes out of scope.

This change was requested by Matt Corallo <git@bluematt.me> and makes code that
sets up mock databases cleaner. The mock database environment will now go out
of scope and be reset on destruction so there is no need to call
BerkeleyEnvironment::Reset() during wallet construction to clear out prior
state.

This change does affect bitcoin behavior slightly. On startup, instead of same
wallet environments staying open throughout VerifyWallets() and OpenWallets()
calls, VerifyWallets() will open and close an environment once for each wallet,
and OpenWallets() will create its own environment(s) later.
  • Loading branch information
@ryanofsky
ryanofsky committed Nov 26, 2018
1 parent b5c3d7a commit f1f4bb7
Show file tree
Hide file tree
Showing 3 changed files with 43 additions and 33 deletions.
39 changes: 21 additions & 18 deletions src/wallet/db.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ void CheckUniqueFileid(const BerkeleyEnvironment& env, const std::string& filena
}

CCriticalSection cs_db;
std::map<std::string, BerkeleyEnvironment> g_dbenvs GUARDED_BY(cs_db); //!< Map from directory name to open db environment.
std::map<std::string, std::weak_ptr<BerkeleyEnvironment>> g_dbenvs GUARDED_BY(cs_db); //!< Map from directory name to db environment.
} // namespace

bool WalletDatabaseFileId::operator==(const WalletDatabaseFileId& rhs) const
Expand Down Expand Up @@ -80,19 +80,22 @@ bool IsWalletLoaded(const fs::path& wallet_path)
LOCK(cs_db);
auto env = g_dbenvs.find(env_directory.string());
if (env == g_dbenvs.end()) return false;
return env->second.IsDatabaseLoaded(database_filename);
auto database = env->second.lock();
return database && database->IsDatabaseLoaded(database_filename);
}

BerkeleyEnvironment* GetWalletEnv(const fs::path& wallet_path, std::string& database_filename)
std::shared_ptr<BerkeleyEnvironment> GetWalletEnv(const fs::path& wallet_path, std::string& database_filename)
{
fs::path env_directory;
SplitWalletPath(wallet_path, env_directory, database_filename);
LOCK(cs_db);
// Note: An unused temporary BerkeleyEnvironment object may be created inside the
// emplace function if the key already exists. This is a little inefficient,
// but not a big concern since the map will be changed in the future to hold
// pointers instead of objects, anyway.
return &g_dbenvs.emplace(std::piecewise_construct, std::forward_as_tuple(env_directory.string()), std::forward_as_tuple(env_directory)).first->second;
auto inserted = g_dbenvs.emplace(env_directory.string(), std::weak_ptr<BerkeleyEnvironment>());
if (inserted.second) {
auto env = std::make_shared<BerkeleyEnvironment>(env_directory.string());
inserted.first->second = env;
return env;
}
return inserted.first->second.lock();
}

//
Expand Down Expand Up @@ -137,6 +140,7 @@ BerkeleyEnvironment::BerkeleyEnvironment(const fs::path& dir_path) : strPath(dir

BerkeleyEnvironment::~BerkeleyEnvironment()
{
g_dbenvs.erase(strPath);
Close();
}

Expand Down Expand Up @@ -214,10 +218,9 @@ bool BerkeleyEnvironment::Open(bool retry)
return true;
}

void BerkeleyEnvironment::MakeMock()
BerkeleyEnvironment::BerkeleyEnvironment()
{
if (fDbEnvInit)
throw std::runtime_error("BerkeleyEnvironment::MakeMock: Already initialized");
Reset();

boost::this_thread::interruption_point();

Expand Down Expand Up @@ -266,7 +269,7 @@ BerkeleyEnvironment::VerifyResult BerkeleyEnvironment::Verify(const std::string&
bool BerkeleyBatch::Recover(const fs::path& file_path, void *callbackDataIn, bool (*recoverKVcallback)(void* callbackData, CDataStream ssKey, CDataStream ssValue), std::string& newFilename)
{
std::string filename;
BerkeleyEnvironment* env = GetWalletEnv(file_path, filename);
std::shared_ptr<BerkeleyEnvironment> env = GetWalletEnv(file_path, filename);

// Recovery procedure:
// move wallet file to walletfilename.timestamp.bak
Expand Down Expand Up @@ -335,7 +338,7 @@ bool BerkeleyBatch::Recover(const fs::path& file_path, void *callbackDataIn, boo
bool BerkeleyBatch::VerifyEnvironment(const fs::path& file_path, std::string& errorStr)
{
std::string walletFile;
BerkeleyEnvironment* env = GetWalletEnv(file_path, walletFile);
std::shared_ptr<BerkeleyEnvironment> env = GetWalletEnv(file_path, walletFile);
fs::path walletDir = env->Directory();

LogPrintf("Using BerkeleyDB version %s\n", DbEnv::version(0, 0, 0));
Expand All @@ -359,7 +362,7 @@ bool BerkeleyBatch::VerifyEnvironment(const fs::path& file_path, std::string& er
bool BerkeleyBatch::VerifyDatabaseFile(const fs::path& file_path, std::string& warningStr, std::string& errorStr, BerkeleyEnvironment::recoverFunc_type recoverFunc)
{
std::string walletFile;
BerkeleyEnvironment* env = GetWalletEnv(file_path, walletFile);
std::shared_ptr<BerkeleyEnvironment> env = GetWalletEnv(file_path, walletFile);
fs::path walletDir = env->Directory();

if (fs::exists(walletDir / walletFile))
Expand Down Expand Up @@ -463,7 +466,7 @@ BerkeleyBatch::BerkeleyBatch(BerkeleyDatabase& database, const char* pszMode, bo
{
fReadOnly = (!strchr(pszMode, '+') && !strchr(pszMode, 'w'));
fFlushOnClose = fFlushOnCloseIn;
env = database.env;
env = database.env.get();
if (database.IsDummy()) {
return;
}
Expand Down Expand Up @@ -520,7 +523,7 @@ BerkeleyBatch::BerkeleyBatch(BerkeleyDatabase& database, const char* pszMode, bo
// versions of BDB have an set_lk_exclusive method for this
// purpose, but the older version we use does not.)
for (const auto& env : g_dbenvs) {
CheckUniqueFileid(env.second, strFilename, *pdb_temp, this->env->m_fileids[strFilename]);
CheckUniqueFileid(*env.second.lock().get(), strFilename, *pdb_temp, this->env->m_fileids[strFilename]);
}

pdb = pdb_temp.release();
Expand Down Expand Up @@ -621,7 +624,7 @@ bool BerkeleyBatch::Rewrite(BerkeleyDatabase& database, const char* pszSkip)
if (database.IsDummy()) {
return true;
}
BerkeleyEnvironment *env = database.env;
BerkeleyEnvironment *env = database.env.get();
const std::string& strFile = database.strFile;
while (true) {
{
Expand Down Expand Up @@ -752,7 +755,7 @@ bool BerkeleyBatch::PeriodicFlush(BerkeleyDatabase& database)
return true;
}
bool ret = false;
BerkeleyEnvironment *env = database.env;
BerkeleyEnvironment *env = database.env.get();
const std::string& strFile = database.strFile;
TRY_LOCK(cs_db, lockDb);
if (lockDb)
Expand Down
34 changes: 19 additions & 15 deletions src/wallet/db.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,10 +50,10 @@ class BerkeleyEnvironment
std::condition_variable_any m_db_in_use;

BerkeleyEnvironment(const fs::path& env_directory);
BerkeleyEnvironment();
~BerkeleyEnvironment();
void Reset();

void MakeMock();
bool IsMock() const { return fMockDb; }
bool IsInitialized() const { return fDbEnvInit; }
bool IsDatabaseLoaded(const std::string& db_filename) const { return m_databases.find(db_filename) != m_databases.end(); }
Expand Down Expand Up @@ -102,7 +102,7 @@ class BerkeleyEnvironment
bool IsWalletLoaded(const fs::path& wallet_path);

/** Get BerkeleyEnvironment and database filename given a wallet path. */
BerkeleyEnvironment* GetWalletEnv(const fs::path& wallet_path, std::string& database_filename);
std::shared_ptr<BerkeleyEnvironment> GetWalletEnv(const fs::path& wallet_path, std::string& database_filename);

/** An instance of this class represents one database.
* For BerkeleyDB this is just a (env, strFile) tuple.
Expand All @@ -117,17 +117,11 @@ class BerkeleyDatabase
}

/** Create DB handle to real database */
BerkeleyDatabase(const fs::path& wallet_path, bool mock = false) :
nUpdateCounter(0), nLastSeen(0), nLastFlushed(0), nLastWalletUpdate(0)
BerkeleyDatabase(std::shared_ptr<BerkeleyEnvironment> env, std::string filename) :
nUpdateCounter(0), nLastSeen(0), nLastFlushed(0), nLastWalletUpdate(0), env(std::move(env)), strFile(std::move(filename))
{
env = GetWalletEnv(wallet_path, strFile);
auto inserted = env->m_databases.emplace(strFile, std::ref(*this));
auto inserted = this->env->m_databases.emplace(strFile, std::ref(*this));
assert(inserted.second);
if (mock) {
env->Close();
env->Reset();
env->MakeMock();
}
}

~BerkeleyDatabase() {
Expand All @@ -140,7 +134,8 @@ class BerkeleyDatabase
/** Return object for accessing database at specified path. */
static std::unique_ptr<BerkeleyDatabase> Create(const fs::path& path)
{
return MakeUnique<BerkeleyDatabase>(path);
std::string filename;
return MakeUnique<BerkeleyDatabase>(GetWalletEnv(path, filename), std::move(filename));
}

/** Return object for accessing dummy database with no read/write capabilities. */
Expand All @@ -152,7 +147,7 @@ class BerkeleyDatabase
/** Return object for accessing temporary in-memory database. */
static std::unique_ptr<BerkeleyDatabase> CreateMock()
{
return MakeUnique<BerkeleyDatabase>("", true /* mock */);
return MakeUnique<BerkeleyDatabase>(std::make_shared<BerkeleyEnvironment>(), "");
}

/** Rewrite the entire database on disk, with the exception of key pszSkip if non-zero
Expand All @@ -176,12 +171,21 @@ class BerkeleyDatabase
unsigned int nLastFlushed;
int64_t nLastWalletUpdate;

/**
* Pointer to shared database environment.
*
* Normally there is only one BerkeleyDatabase object per
* BerkeleyEnvivonment, but in the special, backwards compatible case where
* multiple wallet BDB data files are loaded from the same directory, this
* will point to a shared instance that gets freed when the last data file
* is closed.
*/
std::shared_ptr<BerkeleyEnvironment> env;

/** Database pointer. This is initialized lazily and reset during flushes, so it can be null. */
std::unique_ptr<Db> m_db;

private:
/** BerkeleyDB specific */
BerkeleyEnvironment *env;
std::string strFile;

/** Return whether this database handle is a dummy for testing.
Expand Down
3 changes: 3 additions & 0 deletions src/wallet/wallet.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3877,6 +3877,9 @@ bool CWallet::Verify(interfaces::Chain& chain, const WalletLocation& location, b
return false;
}

// Keep same database environment instance across Verify/Recover calls below.
std::unique_ptr<WalletDatabase> database = WalletDatabase::Create(wallet_path);

try {
if (!WalletBatch::VerifyEnvironment(wallet_path, error_string)) {
return false;
Expand Down

0 comments on commit f1f4bb7

Please sign in to comment.