Not sure if this is the right fix, IIRC the wallet is removed from the list first and then the actual unload happens.

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:

1. add an option to unloadwallet to force waiting 👍 👍
2. use this change but also change listwallets to actually verify if the wallet is not unloaded 👎
3. make loadwallet aware the wallet is unloading and wait for it 👍
4. change the test to repeat the loadwallet until it succeeds 👍 👍 👍 👍

Note that there are 2 wallet "registries":

@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:

diff --git a/test/functional/test_framework/util.py b/test/functional/test_framework/util.py
index d0a78d8df..4af7a51ff 100644
--- a/test/functional/test_framework/util.py
+++ b/test/functional/test_framework/util.py
@@ -201,20 +201,24 @@ def str_to_b64str(string):
 def satoshi_round(amount):
     return Decimal(amount).quantize(Decimal('0.00000001'), rounding=ROUND_DOWN)

-def wait_until(predicate, *, attempts=float('inf'), timeout=float('inf'), lock=None):
+def wait_until(predicate, *, attempts=float('inf'), timeout=float('inf'), lock=None, ignore_expections=False):
     if attempts == float('inf') and timeout == float('inf'):
         timeout = 60
     attempt = 0
     time_end = time.time() + timeout

     while attempt < attempts and time.time() < time_end:
-        if lock:
-            with lock:
+        try:
+            if lock:
+                with lock:
+                    if predicate():
+                        return
+            else:
                 if predicate():
                     return
-        else:
-            if predicate():
-                return
+        except JSONRPCException as e:
+            if not ignore_expections:
+                raise e
         attempt += 1
         time.sleep(0.05)

And then:

wait_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.

@ryanofsky the idea was to not block especially if the wallet is being used or a rescan is going on. In that case the client could timeout and then the problem would be the same — attempt to load would raise Duplicate -wallet filename specified error. But maybe we should ignore these edge cases and make it synchronous or even add an option?

Agree with @ryanofsky. Seems unnecessary to make this call async. If the wallet is in use (e.g. rescan) you should be aware of it and not call unloadwallet. So shouldn't unloadwallet throw an error in that case?

Closing in favor of #14941

@laanwj Please do not close PR:s unless there is inactivity or the author is clearly not cooperating. The authors are perfectly capable of doing this themselves.

@kallewoof I don't think there's a point of having multiple competing PRs open, this spreads discussion around, reopening is always a possibility if discussion goes the other way

I understand your desire to have a clean list of PRs, but there really is no rush on the matter. This PR literally had pushes to it hours before you closed it. If the author judges that the replacement is objectively better they will close their PR on their own, and if they refuse, I see no problem with you making a judgement call like this one.

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.

Removing my comment as it strays off topic from the PR. I've sent it to @laanwj in private.

@kallewoof I don't think it makes sense to change the test tree to later revert. There is already agreement to change unloadwallet and @laanwj just closed this in light of that - being closed doesn't mean is locked and dead.

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.

@kallewoof @promag @ryanofsky @MarcoFalke @laanwj Thank you so much.
Now I understand that we need to fix the behaviour of unloadwallet rather than changing the test.
So, I close this PR myself.

Thank you @AkioNak for identifying the problem so quick.