Skip to content

Commit

Permalink
Free CDBEnv instances when not in use
Browse files Browse the repository at this point in the history 
Instead of adding CDBEnv objects permanently to the g_dbenvs map, use reference
counted shared pointers and remove map entries when the last CDBEnv 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
CDBEnv::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 Jan 16, 2018
1 parent ff2fc86 commit c5c0687
Show file tree
Hide file tree
Showing 3 changed files with 32 additions and 30 deletions.
37 changes: 20 additions & 17 deletions src/wallet/db.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,10 +54,10 @@ void CheckUniqueFileid(const CDBEnv& env, const std::string& filename, Db& db)
}

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

CDBEnv* GetWalletEnv(const fs::path& wallet_path, std::string& database_filename)
std::shared_ptr<CDBEnv> GetWalletEnv(const fs::path& wallet_path, std::string& database_filename)
{
fs::path env_directory;
if (fs::is_regular_file(wallet_path)) {
Expand All @@ -73,11 +73,14 @@ CDBEnv* GetWalletEnv(const fs::path& wallet_path, std::string& database_filename
database_filename = "wallet.dat";
}
LOCK(cs_db);
// Note: An ununsed temporary CDBEnv 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<CDBEnv>());
if (inserted.second) {
auto env = std::make_shared<CDBEnv>(env_directory.string());
inserted.first->second = env;
return env;
} else {
return inserted.first->second.lock();
}
}

//
Expand Down Expand Up @@ -122,6 +125,7 @@ CDBEnv::CDBEnv(const fs::path& dir_path) : strPath(dir_path.string())

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

Expand Down Expand Up @@ -195,10 +199,9 @@ bool CDBEnv::Open(bool retry)
return true;
}

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

boost::this_thread::interruption_point();

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

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

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

if (fs::exists(walletDir / walletFile))
Expand Down Expand Up @@ -444,7 +447,7 @@ CDB::CDB(CWalletDBWrapper& dbw, const char* pszMode, bool fFlushOnCloseIn) : pdb
{
fReadOnly = (!strchr(pszMode, '+') && !strchr(pszMode, 'w'));
fFlushOnClose = fFlushOnCloseIn;
env = dbw.env;
env = dbw.env.get();
if (dbw.IsDummy()) {
return;
}
Expand Down Expand Up @@ -501,7 +504,7 @@ CDB::CDB(CWalletDBWrapper& dbw, const char* pszMode, bool fFlushOnCloseIn) : pdb
// versions of BDB have an set_lk_exclusive method for this
// purpose, but the older version we use does not.)
for (auto& env : g_dbenvs) {
CheckUniqueFileid(env.second, strFilename, *pdb_temp);
CheckUniqueFileid(*env.second.lock().get(), strFilename, *pdb_temp);
}

pdb = pdb_temp.release();
Expand Down Expand Up @@ -574,7 +577,7 @@ bool CDB::Rewrite(CWalletDBWrapper& dbw, const char* pszSkip)
if (dbw.IsDummy()) {
return true;
}
CDBEnv *env = dbw.env;
CDBEnv *env = dbw.env.get();
const std::string& strFile = dbw.strFile;
while (true) {
{
Expand Down Expand Up @@ -704,7 +707,7 @@ bool CDB::PeriodicFlush(CWalletDBWrapper& dbw)
return true;
}
bool ret = false;
CDBEnv *env = dbw.env;
CDBEnv *env = dbw.env.get();
const std::string& strFile = dbw.strFile;
TRY_LOCK(cs_db, lockDb);
if (lockDb)
Expand Down
22 changes: 9 additions & 13 deletions src/wallet/db.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

#include <atomic>
#include <map>
#include <memory>
#include <string>
#include <vector>

Expand All @@ -39,10 +40,10 @@ class CDBEnv
std::map<std::string, Db*> mapDb;

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

void MakeMock();
bool IsMock() const { return fMockDb; }
bool IsInitialized() const { return fDbEnvInit; }
fs::path Directory() const { return strPath; }
Expand Down Expand Up @@ -86,7 +87,7 @@ class CDBEnv
};

/** Get CDBEnv and database filename given a wallet path. */
CDBEnv* GetWalletEnv(const fs::path& wallet_path, std::string& database_filename);
std::shared_ptr<CDBEnv> 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 @@ -101,21 +102,16 @@ class CWalletDBWrapper
}

/** Create DB handle to real database */
CWalletDBWrapper(const fs::path& wallet_path, bool mock = false) :
nUpdateCounter(0), nLastSeen(0), nLastFlushed(0), nLastWalletUpdate(0)
CWalletDBWrapper(std::shared_ptr<CDBEnv> 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);
if (mock) {
env->Close();
env->Reset();
env->MakeMock();
}
}

/** Return object for accessing database at specified path. */
static std::unique_ptr<CWalletDBWrapper> Create(const fs::path& path)
{
return MakeUnique<CWalletDBWrapper>(path);
std::string filename;
return MakeUnique<CWalletDBWrapper>(GetWalletEnv(path, filename), std::move(filename));
}

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

/** Rewrite the entire database on disk, with the exception of key pszSkip if non-zero
Expand All @@ -151,7 +147,7 @@ class CWalletDBWrapper

private:
/** BerkeleyDB specific */
CDBEnv *env;
std::shared_ptr<CDBEnv> env;
std::string strFile;

/** Return whether this database handle is a dummy for testing.
Expand Down
3 changes: 3 additions & 0 deletions src/wallet/init.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -242,6 +242,9 @@ bool VerifyWallets()
return InitError(strprintf(_("Error loading wallet %s. Duplicate -wallet filename specified."), walletFile));
}

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

std::string strError;
if (!CWalletDB::VerifyEnvironment(wallet_path, strError)) {
return InitError(strError);
Expand Down

0 comments on commit c5c0687

Please sign in to comment.