Fix UTXO rpc clients dead lock on balance/unspent requests.

[artemii235]

#846

The problem was in concurrent requests handling. Electrum/Daemon could be easily spammed with a lot of balance/unspents requests when many swaps are started or many orders are placed at once. To avoid this I added the feature to send request only once so all other concurrent callers receive the same response once the request is finished. There was a race condition bug in this implementation - it could be possible that there is no active request at the moment, but the list_unspent_in_progress flag was true, which locked the call forever.

@sergeyboyko0791
There are a couple of unwraps in ConcurrentRequestMap::wrap_request that should not panic, but I don't like them anyway 🙂 Maybe you will have an idea on how to get rid of them?

@cipig @Milerius @tonymorony Could you please give this a test in both Electrum and Native modes? The easiest way to reproduce the issue was to make a lot of balance requests concurrently in the loop and then disconnect the network for a while.

[sergeyboyko0791]

For me, it looks leaner, cleaner, and better than the previous concurrency map implementation.
I've left a few comments, but you can ignore them, cause they don't affect code execution.

Perhaps, these unwraps can be replaced by expect("description") :D

[artemii235]

@sergeyboyko0791 Thanks for your review! I've made the changes, could you recheck, please?

[tonymorony]

LGTM

1. I've reproduced the problem on the old version (manipulations with the connection not needed on my side)

if run this script in a single thread mm2 returns answer fine, if run this script in two threads (e.g. second terminal tab) mm2 stops to response until the restart

balances_spam.sh 
#!/bin/bash
while :
do
  curl --url "http://127.0.0.1:7783" --data "{\"userpass\":\"$userpass\",\"method\":\"my_balance\",\"coin\":\"RICK\"}"
  curl --url "http://127.0.0.1:7783" --data "{\"userpass\":\"$userpass\",\"method\":\"my_balance\",\"coin\":\"MORTY\"}"
done

2. on mm2.1-fix-api-deadlock such spam script works fine in multiple threads

[sergeyboyko0791]

Thanks, nice!

[cipig]

mm2 nodes with hundreds of orders running fine since 24h, previous problems are gone

[cipig]

i fear that 731e2d2 breaks it, i get lots of timeouts after updating to it
5b20ad1 works fine, i was using that for 24h... going back to it solves the timeouts

[artemii235]

i fear that 731e2d2 breaks it, i get lots of timeouts after updating to it

I've done it because Slyris reported a race condition: when he called setprice with cancel_previous:true for the same pair concurrently 2 orders were created instead of 1. We should avoid holding the my_maker_orders mutex during network requests so I moved the balance check code upper. However, it cancels the fix made for #794 because cancellation will happen after the balance check. What should we do about this? 🙁

[artemii235]

@cipig Could you test the latest commit, please? I've added explicit orders cancellation in case of errors.

[cipig]

it works now, no more timeouts, thanks for the fix

[Milerius]

Tested on OSX with my bot, previous order are correctly cancelled now.

[artemii235]

Great, thanks to everyone for the reviews!