wallet_multiwallet.py --usecli may sometimes fails with
“Duplicate -wallet filename specified”
unloadwallet() RPC command just notify the unload intention
and it may delay a little bit until actual unloading.
This patch check the completion of unloading by using listwallets().
This fixes #14917
The relevant code is
The wallet itself has locks to prevent it from being loaded while already loaded, so it seems like an OK fix to me.
@kallewoof that’s what happening, it hits that error.
It’s always the case that after unloadwallet X listwallets doesn’t return X but that wallet can be unloading. This change only reduces the race to the actual duplicate condition.
I see 4 solutions:
unloadwallet to force waiting 👍 👍listwallets to actually verify if the wallet is not unloaded 👎loadwallet aware the wallet is unloading and wait for it 👍loadwallet until it succeeds 👍 👍 👍 👍Note that there are 2 wallet “registries”: https://github.com/bitcoin/bitcoin/blob/5f23460c7e316fe7c944680f3feff07ebb867f70/src/wallet/wallet.cpp#L69-L73 https://github.com/bitcoin/bitcoin/blob/5f23460c7e316fe7c944680f3feff07ebb867f70/src/wallet/db.cpp#L75-L84
@promag @kallewoof Thank you for the discussion.
change the test to repeat the loadwallet until it succeeds 👍 👍 👍 👍
I will try this.
@AkioNak you could add an option to wait_until, untested:
0diff --git a/test/functional/test_framework/util.py b/test/functional/test_framework/util.py
1index d0a78d8df..4af7a51ff 100644
2--- a/test/functional/test_framework/util.py
3+++ b/test/functional/test_framework/util.py
4@@ -201,20 +201,24 @@ def str_to_b64str(string):
5 def satoshi_round(amount):
6 return Decimal(amount).quantize(Decimal('0.00000001'), rounding=ROUND_DOWN)
7
8-def wait_until(predicate, *, attempts=float('inf'), timeout=float('inf'), lock=None):
9+def wait_until(predicate, *, attempts=float('inf'), timeout=float('inf'), lock=None, ignore_expections=False):
10 if attempts == float('inf') and timeout == float('inf'):
11 timeout = 60
12 attempt = 0
13 time_end = time.time() + timeout
14
15 while attempt < attempts and time.time() < time_end:
16- if lock:
17- with lock:
18+ try:
19+ if lock:
20+ with lock:
21+ if predicate():
22+ return
23+ else:
24 if predicate():
25 return
26- else:
27- if predicate():
28- return
29+ except JSONRPCException as e:
30+ if not ignore_expections:
31+ raise e
32 attempt += 1
33 time.sleep(0.05)
And then:
0wait_until(lambda: self.nodes[0].loadwallet(wallet_name), ignore_exceptions=True)
BTW, now I’m more fond of
make loadwallet aware the wallet is unloading and wait for it @ryanofsky what are your thoughts?
make loadwallet aware the wallet is unloading and wait for it
I think a better approach would be to make unloadwallet aware that a wallet is unloading and wait for it. It’s surprising to me that this would be an asynchronous operation.
Duplicate -wallet filename specified error. But maybe we should ignore these edge cases and make it synchronous or even add an option?
wallet_multiwallet.py --usecli may sometimes fails with
"Duplicate -wallet filename specified"
Unloadwallet RPC command just notify the unload intention
and it may delay a little bit until actual unloading.
This patch make loadwallet to retry if JSONRPCException occurs.
I’m not sure how you’re interpreting it, but for me, closing the PR is only an indication that it’s currently not considered for merging. It doesn’t delete the discussion, doesn’t even prevent replying! it doesn’t delete the code changes, etc.
That’s a typical maintainer task. at least for me it has nothing to do with ‘urgency’ or capability or non-capability of the author to do it themselves.
But anyhow if you insist I’ll re-open.
Yes, so I think there’s a misunderstanding here: Thanks @AkioNak for trying to fix the test!
However your PR made us realize that the current unloadwallet behavior is broken in the first place and that the unload shouldn’t be asynchronous, and the test failure is pointing out an actual issue. It likely means that the test doesn’t need to change.
unloadwallet rather than changing the test.
So, I close this PR myself.