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.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
Concept ACK - I think this can be useful outside of the tests, as well.
Also I’d like to discuss the wait default value, currently it avoid changing behavior. If wait=true is considered the correct behavior then this could be backport to 0.17.2?
Strictly spoken this is a feature, not a bugfix, though if it’s optional and improves testing I think a point can be made to backport it anyhow.
Code change looks ok, but this shouldn’t be an option. Making wait=false the default makes unloadwallet pointlessly difficult to use, and inconsistent with other methods, including closely related methods like createwallet and loadwallet.
If you think there is reason to support wait=false, I think you need to say what the use-case is, and why it wouldn’t be better served by having the client make a fully asynchronous JSON-RPC request, instead of using this half-asynchronous/half-synchronous wait option that doesn’t (I guess?) block on acquiring cs_wallet, but does block on the network connection and on acquiring cs_wallets, and on whatever the UI currently does in NotifyUnload (or will do in the future).
Also, if you want to keep the async=false option, it would be good to have a test to ensure that it doesn’t acquire cs_wallet and cs_main. This would be easy to write as a c++ unit test which acquires the locks in one thread and calls unloadwallet in another.
Changed to synchronous unload. Locally added some sleeps to see if there were any connection timeout, but didn’t manage to cause one.
doesn’t (I guess?) block on acquiring
cs_wallet
no it doesn’t, it waits until the weak pointer is expired.
102- wallet->WalletLogPrintf("Releasing wallet\n");
103- wallet->BlockUntilSyncedToCurrentChain();
104- wallet->Flush();
105- delete wallet;
106+ {
107+ std::unique_lock<std::mutex> lk(g_wallet_release_mutex);
sync.h instead of mutex and unique_lock for thread annotations & debugging.
80@@ -81,13 +81,33 @@ std::shared_ptr<CWallet> GetWallet(const std::string& name)
81 return nullptr;
82 }
83
84+static std::mutex g_wallet_release_mutex;
85+static std::condition_variable g_wallet_release_cond;
cv instead of cond and name locks lock instead of lk.
109+ wallet->BlockUntilSyncedToCurrentChain();
110+ wallet->Flush();
111+ delete wallet;
112+ }
113+
114+ g_wallet_release_cond.notify_all();
!weak_wallet.expired() in the other thread is true at the point of this call (and guaranteed by the c++ standard to be true). Otherwise there could be a lost-wakeup bug here because this would be signalling the condition variable before the condition is true.
88+{
89+ std::weak_ptr<CWallet> weak_wallet(wallet);
90+ wallet.reset();
91+
92+ std::unique_lock<std::mutex> lk(g_wallet_release_mutex);
93+ while (!weak_wallet.expired()) {
It looks like this might not be sufficient if we want to guarantee that the wallet is really destroyed. From https://en.cppreference.com/w/cpp/memory/weak_ptr/expired:
The destructor for the managed object may not yet have been called, but this object’s destruction is imminent (or may have already happened).
A more reliable but cumbersome approach might be to add wallet members:
0std::promise<void> m_unload_promise;
1std:shared_future<void> m_unload_future = m_unload_promise.get_future();
and call m_unloaded_promise.set_value() in the destructor, and then future.wait() here.
120+ g_wallet_release_cv.wait(lock);
121+ }
122+ }
123+ // Now it's safe to close and delete the wallet.
124+ pwallet->WalletLogPrintf("Releasing wallet\n");
125+ pwallet->BlockUntilSyncedToCurrentChain();
UnregisterValidationInterface otherwise SyncWithValidationInterfaceQueue does nothing than waiting..
53@@ -54,6 +54,11 @@ void StopWallets();
54 //! Close all wallets.
55 void UnloadWallets();
56
57+//! Unloads and deletes the wallet.
58+// Blocks current thread until the wallet is destroyed.
59+// The caller must std::move the pointer otherwise it deadlocks
std::shared_ptr<CWallet>&& to make the compiler require an rvalue.
261@@ -262,10 +262,10 @@ void Shutdown(InitInterfaces& interfaces)
262 LogPrintf("%s: Unable to remove pidfile: %s\n", __func__, e.what());
263 }
264 #endif
265+ interfaces.chain_clients.clear();
94- wallet->Flush();
95- delete wallet;
96+ {
97+ std::unique_lock<std::mutex> lock(g_wallet_release_mutex);
98+ // Assert that there was an explict UnloadWallet call.
99+ assert(g_unloading_wallet_set.erase(wallet) == 1);
Would suggest:
0size_t erased = g_unloading_wallet_set.erase(wallet) == 1
1assert(erased == 1);
Even though we don’t support building bitcoin with NDEBUG, I think most people just expect asserts to contain error checking, not code that is important to run.
This change seems like it should work, but multiwallet test is failing. There’s some problem with combine_logs on travis, but locally I see:
0AssertionError: [node 0] Expected message "BerkeleyBatch: Can't open database w8_copy \(duplicates fileid \w+ from w8\)" does not partially match stderr:
1"bitcoind: wallet/wallet.cpp:95: void ReleaseWallet(CWallet*): Assertion `g_unloading_wallet_set.erase(wallet) == 1' failed."
104+ {
105+ LOCK(g_wallet_release_mutex);
106+ unloading = g_unloading_wallet_set.erase(wallet) == 1;
107+ }
108+ if (unloading) {
109+ // Delegate wallet deletion for UnloadWallet caller.
Delegate wallet deletion to UnloadWallet caller
107+ }
108+ if (unloading) {
109+ // Delegate wallet deletion for UnloadWallet caller.
110+ g_wallet_release_cv.notify_all();
111+ } else {
112+ // Delegate wallet deletion for UnloadWallet caller.
NotifyUnload because at this point there are no shared pointers to this wallet.
100 }
101
102+// Custom deleter for shared_ptr<CWallet>.
103+static void ReleaseWallet(CWallet* wallet)
104+{
105+ bool unloading{false};
Is there a reason this needs to delegate deletion? It seems if you removed the DeleteWallet call from UnloadWallet, you could simplify this function to just:
0DeleteWallet(wallet);
1LOCK(g_wallet_release_mutex);
2g_unloading_wallet_set.erase(wallet);
3g_wallet_release_cv.notify_all();
unloadwallet caller. The way you suggest both the unloadwallet caller and the last wallet client wait for the complete unload.
just thought the unload burden/time should be left to the unloadwallet caller.
Interesting. That seems like a legitimate optimization for the last RPC using the wallet. Personally, I think I’d still choose the simplification over the optimization, but if you want to keep the optimization it’d be good to have a comment saying why it’s better to delete on the unload thread than the release thread.
53@@ -54,6 +54,10 @@ void StopWallets();
54 //! Close all wallets.
55 void UnloadWallets();
56
57+//! Unloads and deletes the wallet.
58+// Blocks current thread until the wallet is deleted.
59+void UnloadWallet(std::shared_ptr<CWallet>&& wallet);
126+ assert(g_unloading_wallet_set.insert(pwallet).second);
127+ }
128+ // The wallet can be in use so it's not possible to explicitly unload here.
129+ // Notify the unload intent so that all remaining shared pointers are
130+ // released.
131+ pwallet->NotifyUnload();
assert(NotifyUnload.empty()) in the wallet destructor to verify delete isn’t happening too early and clients are fully detached?
98+ UnregisterValidationInterface(wallet);
99 delete wallet;
100 }
101
102+// Custom deleter for shared_ptr<CWallet>.
103+// If the release is due to a UnloadWallet call then deletion is delegated to
This comment needs to be updated with the latest change.
Also nit: s/a UnloadWallet/an UnloadWallet/
113- } else {
114- // UnloadWallet was not called, just delete the wallet since there's no
115- // more shared pointers to the wallet.
116- DeleteWallet(wallet);
117- }
118+ DeleteWallet(wallet);
DeleteWallet here. This function is only called from this one place, and I can’t think of any other place where it would be safe to call.