In order to test the new check and error add:

--- a/src/rpc/blockchain.cpp
+++ b/src/rpc/blockchain.cpp
@@ -1619,6 +1619,8 @@ UniValue savemempool(const JSONRPCRequest& request)
         throw JSONRPCError(RPC_MISC_ERROR, "Currently dumping the mempool");
     }

+    MilliSleep(10000);
+
     if (!g_is_mempool_loaded) {
         throw JSONRPCError(RPC_MISC_ERROR, "The mempool was not loaded yet");
     }

Then run

bitcoin-cli -regtest savemempool & bitcoin-cli -regtest savemempool

The second should wait for the first to terminate.

After #12863 I'm even more convinced that this might be the wrong approach. Protecting access using a mutex instead of returning these kind of 'busy errors' would avoid having to implement retry-poll-loops client-side (including in the tests).

If you want to stick with this you'd at least need to define a new error code that means 'transient error', similar to RPC_IN_WARMUP. But I think pushing this responsibility to the client is unnecessary.

@laanwj indeed it's very arguable. I think it's preferable to have the client retrying than reserving resources on the server side. IMO having the client waiting is also not desirable because the client can wait indefinitely and also timeout (but then he can raise the timeout) and we would process the request anyway (?). Getting an error instead of waiting is also more informative and allows the client to explicitly log and retry when he feels like.

OTOH

• Then the API wouldn't be changed. Users of savemempool can rely on the call working, or fatally failing.
• Hanging indefinitely is a bug no matter what. If it takes too long, something is wrong.
• Interrupt semantics usually require less resources than polling. In this case: sleeping on a mutex requires virtually no resources. Whereas repetitive RPC requests take up CPU cycles and I/O.

I think we had reports that loading the mempool took 1.5 hours or so, just noting without further comment.

Then the API wouldn't be changed. Users of savemempool can rely on the call working, or fatally failing.

The caller should always have error handling, and this new error would fall in his "unkown error".

Hanging indefinitely is a bug no matter what. If it takes too long, something is wrong.

We have calls that can hang a lot of time, for instance when the wallet is really big (as in lots of keys, lots of transactions).

Interrupt semantics usually require less resources than polling.

Right, but at least a thread and a socket is on hold. And when the mutex is acquired, it might do something that is no longer relevant - in this case dumping the mempool again..

We have this semantic in rescanblockchain, do you think we should change that too?

And regarding #12863, having the client blindingly wait for 1.5 hours should not be an option (?)

I'm also more a fan of blocking until the existing save has finished. Given that no modifications in the mempool can happen in the mean time anyway (I think?), we could even just wait until the existing save operation finishes and then return from both RPC calls (without an additional save).

Given that no modifications in the mempool can happen in the mean time anyway

Actually it can, DumpMempool takes a copy of the mempool contents and then releases the lock to then write to file. So, while writing to disk, the mempool can change.

What should be done here? I've to address @conscott comment above, but I guess I should wait for more concept ACKs.

I agree with @laanwj - it should just hold a lock on the file to avoid problems, but otherwise succeed.

Rebased and updated to block/wait while there is another dump.

@luke do you think the current code is enough?

utACK 35bdaf7489cf33d120b3c66f87067c2698ed2e8c

@MarcoFalke @achow101 rebased on master and changed to Mutex/LOCK and static variable in DumpMempool.

re-utACK 585b47c

I really like this clean and simple solution

utACK 585b47c