From 39308f017a217d9b4bb7f2026632b2d4a882f4b0 Mon Sep 17 00:00:00 2001 From: "Wladimir J. van der Laan" Date: Tue, 15 Jan 2019 14:08:26 +0100 Subject: [PATCH] Merge #14941: rpc: Make unloadwallet wait for complete wallet unload MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 645e905c327411555073fa7964b36f652998059f doc: Add release notes for unloadwallet change to synchronous call (João Barbosa) c37851de5752f107c16e19317f28038b6b7ca2dc rpc: Make unloadwallet wait for complete wallet unload (João Barbosa) Pull request description: Currently the `unloadwallet` RPC is asynchronous, it only signals the intent to unload the wallet and then returns the response to the client. The actual unload can happen later and the client has no way to be notified of that. This PR makes the `unloadwallet` RPC synchronous, meaning that it blocks until the wallet is fully unloaded. Replaces #14919, fixes #14917. Tree-SHA512: ad88b980e2f3652809a58f904afbfe020299f3aa6a517f495ba943b8d54d4520f6e70074d6749be8f5967065c0f476e0faedcde64c8b4899e5f99c70f0fd6534 --- doc/release-notes-14941.md | 5 +++++ src/init.cpp | 2 +- src/wallet/init.cpp | 8 ++++++-- src/wallet/rpcwallet.cpp | 10 +--------- src/wallet/wallet.cpp | 38 ++++++++++++++++++++++++++++++++++++++ src/wallet/wallet.h | 9 +++++++++ 6 files changed, 60 insertions(+), 12 deletions(-) create mode 100644 doc/release-notes-14941.md diff --git a/doc/release-notes-14941.md b/doc/release-notes-14941.md new file mode 100644 index 0000000000..c3820d0368 --- /dev/null +++ b/doc/release-notes-14941.md @@ -0,0 +1,5 @@ +Miscellaneous RPC changes +------------ + +- The `unloadwallet` RPC is now synchronous, meaning that it blocks until the + wallet is fully unloaded. diff --git a/src/init.cpp b/src/init.cpp index a9d7e8e608..8ff72b1f04 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -375,6 +375,7 @@ void PrepareShutdown() LogPrintf("%s: Unable to remove PID file: %s\n", __func__, e.what()); } #endif + g_wallet_init_interface.Close(); UnregisterAllValidationInterfaces(); GetMainSignals().UnregisterBackgroundSignalScheduler(); GetMainSignals().UnregisterWithMempoolSignals(mempool); @@ -396,7 +397,6 @@ void Shutdown() PrepareShutdown(); } // Shutdown part 2: delete wallet instance - g_wallet_init_interface.Close(); globalVerifyHandle.reset(); ECC_Stop(); LogPrintf("%s: done\n", __func__); diff --git a/src/wallet/init.cpp b/src/wallet/init.cpp index eec3396cfc..41ee1062c4 100644 --- a/src/wallet/init.cpp +++ b/src/wallet/init.cpp @@ -402,8 +402,12 @@ void WalletInit::Stop() const void WalletInit::Close() const { - for (const std::shared_ptr& pwallet : GetWallets()) { - RemoveWallet(pwallet); + auto wallets = GetWallets(); + while (!wallets.empty()) { + auto wallet = wallets.back(); + wallets.pop_back(); + RemoveWallet(wallet); + UnloadWallet(std::move(wallet)); } } diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 0d23aa9e36..bb0b1512b8 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -3487,16 +3487,8 @@ static UniValue unloadwallet(const JSONRPCRequest& request) if (!RemoveWallet(wallet)) { throw JSONRPCError(RPC_MISC_ERROR, "Requested wallet already unloaded"); } - UnregisterValidationInterface(wallet.get()); - // The wallet can be in use so it's not possible to explicitly unload here. - // Just notify the unload intent so that all shared pointers are released. - // The wallet will be destroyed once the last shared pointer is released. - wallet->NotifyUnload(); - - // There's no point in waiting for the wallet to unload. - // At this point this method should never fail. The unloading could only - // fail due to an unexpected error which would cause a process termination. + UnloadWallet(std::move(wallet)); return NullUniValue; } diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index a4ac6784e9..3066a02b6b 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -98,14 +98,52 @@ std::shared_ptr GetWallet(const std::string& name) return nullptr; } +static Mutex g_wallet_release_mutex; +static std::condition_variable g_wallet_release_cv; +static std::set g_unloading_wallet_set; // Custom deleter for shared_ptr. static void ReleaseWallet(CWallet* wallet) { + // Unregister and delete the wallet right after BlockUntilSyncedToCurrentChain + // so that it's in sync with the current chainstate. wallet->WalletLogPrintf("Releasing wallet\n"); wallet->BlockUntilSyncedToCurrentChain(); wallet->Flush(); + UnregisterValidationInterface(wallet); delete wallet; + // Wallet is now released, notify UnloadWallet, if any. + { + LOCK(g_wallet_release_mutex); + if (g_unloading_wallet_set.erase(wallet) == 0) { + // UnloadWallet was not called for this wallet, all done. + return; + } + } + g_wallet_release_cv.notify_all(); +} + +void UnloadWallet(std::shared_ptr&& wallet) +{ + // Mark wallet for unloading. + CWallet* pwallet = wallet.get(); + { + LOCK(g_wallet_release_mutex); + auto it = g_unloading_wallet_set.insert(pwallet); + assert(it.second); + } + // The wallet can be in use so it's not possible to explicitly unload here. + // Notify the unload intent so that all remaining shared pointers are + // released. + pwallet->NotifyUnload(); + // Time to ditch our shared_ptr and wait for ReleaseWallet call. + wallet.reset(); + { + WAIT_LOCK(g_wallet_release_mutex, lock); + while (g_unloading_wallet_set.count(pwallet) == 1) { + g_wallet_release_cv.wait(lock); + } + } } const uint256 CMerkleTx::ABANDON_HASH(uint256S("0000000000000000000000000000000000000000000000000000000000000001")); diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index ba9bb6e21e..b51b5c03ef 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -37,6 +37,13 @@ #include #include +//! Explicitly unload and delete the wallet. +// Blocks the current thread after signaling the unload intent so that all +// wallet clients release the wallet. +// Note that, when blocking is not required, the wallet is implicitly unloaded +// by the shared pointer deleter. +void UnloadWallet(std::shared_ptr&& wallet); + bool AddWallet(const std::shared_ptr& wallet); bool RemoveWallet(const std::shared_ptr& wallet); bool HasWallets(); @@ -891,6 +898,8 @@ public: ~CWallet() { + // Should not have slots connected at this point. + assert(NotifyUnload.empty()); delete encrypted_batch; encrypted_batch = nullptr; }