Use m̶u̶l̶t̶i̶p̶r̶o̶c̶e̶s̶s̶i̶n̶g̶ asyncio locks instead of db locks #256
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #256 +/- ##
==========================================
+ Coverage 56.03% 56.32% +0.28%
==========================================
Files 42 42
Lines 3346 3320 -26
==========================================
- Hits 1875 1870 -5
+ Misses 1471 1450 -21
☔ View full report in Codecov by Sentry. |
cashu/mint/ledger.py
Outdated
| self.locks[hash] = ( | ||
| self.locks.get(hash) or mp.Lock() | ||
| ) # create a new lock if it doesn't exist |
There was a problem hiding this comment.
Could make self.locks a defaultdict so it creates for you?
There was a problem hiding this comment.
Seems like defaultdict doesn't accept classes by default (hehe) and would need some additional boilerplate: https://stackoverflow.com/questions/32932494/defaultdict-of-a-class-get-index-key-of-current-called-instance-inside-a-class
cashu/mint/ledger.py
Outdated
| @@ -25,6 +27,9 @@ | |||
|
|
|||
|
|
|||
| class Ledger: | |||
| locks: Dict[str, LockBase] = {} # holds multiprocessing locks | |||
| proofs_pending_lock: LockBase = mp.Lock() # holds locks for proofs_pending database | |||
There was a problem hiding this comment.
Do you know if there's precedent for using multiprocessing.Lock with async? Concern here would be the usual that if it's blocking it'll cause similar async problems/thread-lockups to the blocking requests.
There's also asyncio.Lock https://docs.python.org/3/library/asyncio-sync.html
There was a problem hiding this comment.
Thanks for the review! I chose to not use asyncio.Lock (I used it before this PR) because eventually we want to be able to run the nutshell mint on multiple workers and only the multiprocessing lock allows you do lock across different threads. See here: https://stackoverflow.com/questions/18213619/sharing-a-lock-between-gunicorn-workers
There was a problem hiding this comment.
I don't know if there is any interference between the two though, I will give it a test and see if the lock actually works.
There was a problem hiding this comment.
If it does turn out to be blocking, you can always run in an executor https://docs.python.org/3/library/asyncio-dev.html#running-blocking-code -- adds a bit of overhead but would work
There was a problem hiding this comment.
It was indeed blocking. Once two threads ran into the lock, the whole thing stops. I reverted to asyncio locks for now but this won't be usable if we use multiprocessing in the future.
callebtc
changed the title
cashu/mint/ledger.py
Outdated
| async with self.locks[hash]: | ||
| async with self.db.connect() as conn: | ||
| # will raise an exception if the invoice is not paid | ||
| await self._check_lightning_invoice(amount, hash, conn) |
There was a problem hiding this comment.
Should you clean up self.locks once the invoice has been paid? Otherwise it'll start filling up, which is probably OK but unnecessary.
Would probably need to do this with the lock acquired, then remove it from the dict and let the function return clean up the lock itself when it isn't referenced anymore
There was a problem hiding this comment.
I now delete the locks after they're released, I think that should be safe.
No description provided.