Skip to content

Fix deadlock in safe_bank_fine_grained.py #12

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

wshanks
Copy link

@wshanks wshanks commented Jul 6, 2020

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 mikeckennedy merged commit a069745 into talkpython:master Jan 24, 2022
@mikeckennedy
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants