search

talkpython / async-techniques-python-course

Async Techniques and Examples in Python Course
https://training.talkpython.fm/courses/explore_async_python/async-in-python-with-threading-and-multiprocessing
MIT License
451 stars 249 forks source link

Fix deadlock in safe_bank_fine_grained.py #12

Closed wshanks closed 2 years ago

wshanks commented 4 years ago

I just realized I have had this PR sitting open in my fork for over year...oops :). I hope it is still useful to someone out there.

For me, safe_bank_fine_grained.py deadlocks because, in between lock1 and lock2 in do_transfer(), validate_bank() in another thread starts grabbing all the locks and tries to (and does) grab lock2 before trying to get lock1.

https://github.com/willsALMANJ/async-techniques-python-course/blob/49c9affc958be9c2d69c09a9fc1d22b70aa3b590/src/06-thread-safety/safe_bank_fine_grained.py#L59-L70

https://github.com/willsALMANJ/async-techniques-python-course/blob/49c9affc958be9c2d69c09a9fc1d22b70aa3b590/src/06-thread-safety/safe_bank_fine_grained.py#L90-L94

The fact that you didn't see this deadlock must be due to some architecture/implementation detail -- I guess for you create_accounts() always produces objects with sorted memory addresses whereas for me it usually doesn't (because it deadlocks pretty much every time for me). I used the trick you taught me about sorting lock1 and lock2 in do_transfer() to fix the problem in validate_bank(), so good job teaching :)

If you don't mind, I have one related but off topic question/comment (what is the best forum for such questions? email?): why do you talk about the relative performance of the global and fine grained locking bank examples? The only work being done is adding and subtracting which is CPU bound, so I don't see where threading could gain any performance benefit. I would have thought you would need to do something like make the sleep in do_transfer() have a finite value in order to see a speed up with threading. I tried making the sleep 10 ms and cutting the number of transfers down to 100 in do_bank_stuff(). When I did that, the global locking version took 5 seconds (5 thread x 100 transfers x 10 ms) and the fine grained version took 4.6 seconds, so I saw a little speed up. I tried removing validate_bank() from do_transfer() to see how things would change. The global version still took 5 seconds and the fine grained version took 3.2 seconds -- a bit of speed up since validate_bank() was not locking up all the threads intermittently. Thinking about real world scenarios -- validate_bank() would probably also take a finite time and that would reduce the speed advantage of the fine grained version.

mikeckennedy commented 2 years ago

Thank you @wshanks You're totally right I should have sorted the locks so they are taken in the same order. I also don't know why it worked for me before -- it doesn't on my current M1 mini + Monterey.

In terms of speeding things up with finer grained locks, it depends on what you're scaling. If we ever see Sam Gross's work adopted, then it will be more beneficial I agree. But if you're scaling things that have some distributed aspect, then you'll definitely see a speed up (microservices, database calls, file IO, etc). But if it's just pure computation with current GIL, then no, I agree.

In the example, I didn't go to the complexity of creating a remote DB server and all but in principle that was what the bank account idea was mean to simulate.

Cheers and sorry for the delayed response.