Skip to content
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

Fix deadlock in safe_bank_fine_grained.py #12

Open
wants to merge 1 commit into
base: master
from

Conversation

@willsALMANJ
Copy link

willsALMANJ 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.

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

Successfully merging this pull request may close these issues.

None yet

1 participant
You can’t perform that action at this time.