bpo-32734: Fix asyncio.Lock multiple acquire safety issue #5466

bharel · 2018-01-31T23:48:05Z

As promised, found and fixed in the same day due to the high importance.
This will cause almost non-existing overhead (one try statement) and eliminate the bug.

https://bugs.python.org/issue32734

asvetlov

Looks good, the change is correct.

Please fix minor issues for test.

asvetlov · 2018-02-01T09:05:54Z

Lib/test/test_asyncio/test_locks.py

+            t1.cancel()
+            lock.release()
+
+        asyncio.Task(lockandtrigger(), loop=self.loop)


Please use loop.create_task() for a task creation

Done. Keep in mind asyncio.Task was used in the rest of the tests so I used it in order to be consistent.

asvetlov · 2018-02-01T09:05:54Z

Lib/test/test_asyncio/test_locks.py

+
+        # Make sure only one lock was taken
+        self.assertEqual(lock_count, 1)
+


Please explicitly make sure that all four tasks are finished at the end of test

bedevere-bot · 2018-02-01T09:05:56Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

bedevere-bot · 2018-02-01T17:14:44Z

GH-5477 is a backport of this pull request to the 3.6 branch.

bedevere-bot · 2018-02-01T17:20:12Z

GH-5479 is a backport of this pull request to the 3.7 branch.

bharel · 2018-02-01T17:25:50Z

Alright. Done with changes and backports. Anything else I can help with? :-)

I have made the requested changes; please review again

bedevere-bot · 2018-02-01T17:31:20Z

Thanks for making the requested changes!

@asvetlov: please review the changes made to this pull request.

1st1 · 2018-02-02T04:56:06Z

Lib/asyncio/locks.py

@@ -181,18 +181,22 @@ def locked(self):
            self._locked = True
            return True

+        _waiters = self._waiters


Nit: just use self._waiters. No need for a new variable.

New variable was created in order to avoid the double attribute lookup, I'll remove it if you think it's more readable though.

1st1 · 2018-02-02T04:58:32Z

Lib/asyncio/locks.py

-            await fut
-            self._locked = True
-            return True
+            try:


This is very subtle. Can you add a comment why we use two nested 'try' statements here? I.e. that it's important that 'finally' block is called before 'except CancelledError'

Sure thing :-)

1st1 · 2018-02-02T04:59:40Z

Lib/asyncio/locks.py

-            if not fut.done():
-                fut.set_result(True)
-                break
+        """Wake up the first waiter if it isn't done."""


What if it is done? Can there be a situation where you have [fut, fut] in the _waiters list?

There can be indeed, when 2 or more locks are waiting on acquire.
If the first future is done, it means that later on it will either get the lock, or it was cancelled in which case it will wake up the next waiter upon loop's next cycle.
The patch fixes the bug by avoiding multiple done waiters at once and cleaning up the waiters list asap.
The only way multiple done waiters can happen is if 2 futures get cancelled in which case only 1 waiter will wake up, and the second cancelled future will not wake up another one as lock would have already been taken (if not self._locked: self._wake_up_first()) or it wasn't taken yet it which case it's still sitting .done() in the waiter's list.

In summary:
.done necessarily means either the lock is immediately taken next loop or if it isn't, (.done occurred because of cancellation) next one wakes up. So if we have a done future not including the one currently running we shouldn't wake up another one as it will cause a double wakeup, hence, the almighty bug.

OK, makes sense. Could you please add this all as a comment to _wake_up_first? :)

Phew, now that's a long comment, lemme see how can I shorten it while still managing to fully explain it. Challenge accepted :P

Sure, if you have time to write a short comment :)

Sorry for being anal about this. Every once in a while we find a new bug in asyncio.Lock or asyncio.Queue and it's always a challenge to wrap your head around how they are actually working. Every line of code is significant, sometimes it's not safe to do a simple refactoring because things break in very non-obvious ways.

Nah, all good mate. It took some time to wrap my head around it as well.
Luckily I didn't actually encounter the bug at production but rather found it by reading the implementation which took a couple of hours ;-)

How's this comment? Attempting to be short and to the point as much as you can with synchronization.

1st1

I've left a few review comments

bedevere-bot · 2018-02-02T05:04:57Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

And if you don't make the requested changes, you will be put in the comfy chair!

bharel · 2018-02-02T08:32:22Z

Lib/asyncio/locks.py

@@ -181,18 +181,22 @@ def locked(self):
            self._locked = True
            return True

+        _waiters = self._waiters


New variable was created in order to avoid the double attribute lookup, I'll remove it if you think it's more readable though.

bharel · 2018-02-02T08:32:22Z

Lib/asyncio/locks.py

-            if not fut.done():
-                fut.set_result(True)
-                break
+        """Wake up the first waiter if it isn't done."""


There can be indeed, when 2 or more locks are waiting on acquire.
If the first future is done, it means that later on it will either get the lock, or it was cancelled in which case it will wake up the next waiter upon loop's next cycle.
The patch fixes the bug by avoiding multiple done waiters at once and cleaning up the waiters list asap.
The only way multiple done waiters can happen is if 2 futures get cancelled in which case only 1 waiter will wake up, and the second cancelled future will not wake up another one as lock would have already been taken (if not self._locked: self._wake_up_first()) or it wasn't taken yet it which case it's still sitting .done() in the waiter's list.

In summary:
.done necessarily means either the lock is immediately taken next loop or if it isn't, (.done occurred because of cancellation) next one wakes up. So if we have a done future not including the one currently running we shouldn't wake up another one as it will cause a double wakeup, hence, the almighty bug.

bharel · 2018-02-02T08:32:22Z

Lib/asyncio/locks.py

+            try:
+                await fut
+            finally:
+                _waiters.remove(fut)
        except futures.CancelledError:


@1st1 @asvetlov I'll be honest, the only problem I have (which originates from the original code anyway) is what will happen if the exception isn't a CancelledError (such as task.__step(self, exc=OutOfCoconutsException)). Apart from Monty Python having no horse, the code will enter a deadlock as no one will wake up the next waiter. I'm not entirely sure what exceptions can pass into the task's __step as task doesn't allow set_exception but still better be safe than sorry no?

bharel · 2018-02-02T08:32:22Z

Lib/asyncio/locks.py

-            await fut
-            self._locked = True
-            return True
+            try:


Sure thing :-)

bharel · 2018-02-02T08:48:10Z

I have made the requested changes; please review again

bedevere-bot · 2018-02-02T08:48:12Z

Thanks for making the requested changes!

@1st1, @asvetlov: please review the changes made to this pull request.

asvetlov

LGTM.
@1st1 please review again

bharel · 2018-02-02T21:54:23Z

Alright, I guess that's a green light for my backports?

1st1 · 2018-02-02T22:03:39Z

Alright, I guess that's a green light for my backports?

Let's see if our bots can do them automagically first ;)

miss-islington · 2018-02-02T22:04:03Z

Thanks @bharel for the PR, and @1st1 for merging it 🌮🎉.. I'm working now to backport this PR to: 3.6, 3.7.
🐍🍒⛏🤖

) (cherry picked from commit 2f79c01) Co-authored-by: Bar Harel <bzvi7919@gmail.com>

bedevere-bot · 2018-02-02T22:05:11Z

GH-5501 is a backport of this pull request to the 3.7 branch.

miss-islington · 2018-02-02T22:06:06Z

Sorry, @bharel and @1st1, I could not cleanly backport this to 3.6 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 2f79c014931cbb23b08a7d16c534a3cc9607ae14 3.6

bedevere-bot · 2018-02-02T22:09:17Z

GH-5502 is a backport of this pull request to the 3.6 branch.

bharel · 2018-02-02T22:12:48Z

Thanks @1st1 :-)
Done with 3.6 backport.
Btw, doesn't it apply to 3.5 as well or did we stop supporting it?

1st1 · 2018-02-02T22:16:02Z

3.5 is in security-only fixes mode IIRC.

…5501) (cherry picked from commit 2f79c01) Co-authored-by: Bar Harel <bzvi7919@gmail.com>

) (#5502) (cherry picked from commit d41e9e0)

32734

0fcf18e

bharel requested review from 1st1 and asvetlov as code owners Jan 31, 2018

the-knights-who-say-ni added the CLA signed label Jan 31, 2018

bedevere-bot added the awaiting review label Jan 31, 2018

Better explanation

d54a5f0

asvetlov added needs backport to 3.6 needs backport to 3.7 labels Feb 1, 2018

asvetlov requested changes Feb 1, 2018

View changes

bedevere-bot removed the awaiting review label Feb 1, 2018

bedevere-bot added the awaiting changes label Feb 1, 2018

fix according to CR

195bfea

bedevere-bot removed the needs backport to 3.6 label Feb 1, 2018

bedevere-bot removed the needs backport to 3.7 label Feb 1, 2018

bedevere-bot added awaiting change review and removed awaiting changes labels Feb 1, 2018

1st1 reviewed Feb 2, 2018

View changes

1st1 requested changes Feb 2, 2018

View changes

bedevere-bot added awaiting changes and removed awaiting change review labels Feb 2, 2018

bharel commented Feb 2, 2018

View changes

Added explanation comment

b5d1f0a

bedevere-bot added awaiting change review and removed awaiting changes labels Feb 2, 2018

asvetlov approved these changes Feb 2, 2018

View changes

bedevere-bot added awaiting merge and removed awaiting change review labels Feb 2, 2018

More explanation :P

d41e9e0

1st1 approved these changes Feb 2, 2018

View changes

1st1 added needs backport to 3.6 needs backport to 3.7 labels Feb 2, 2018

1st1 merged commit 2f79c01 into python:master Feb 2, 2018

bedevere-bot removed the awaiting merge label Feb 2, 2018

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Feb 2, 2018

bpo-32734: Fix asyncio.Lock multiple acquire safety issue (pythonGH-5466

dd25e9a

) (cherry picked from commit 2f79c01) Co-authored-by: Bar Harel <bzvi7919@gmail.com>

bedevere-bot removed the needs backport to 3.7 label Feb 2, 2018

bedevere-bot removed the needs backport to 3.6 label Feb 2, 2018

1st1 pushed a commit that referenced this pull request Feb 2, 2018

bpo-32734: Fix asyncio.Lock multiple acquire safety issue (GH-5466) (#…

2b5937e

…5501) (cherry picked from commit 2f79c01) Co-authored-by: Bar Harel <bzvi7919@gmail.com>

1st1 pushed a commit that referenced this pull request Feb 2, 2018

[3.6] bpo-32734: Fix asyncio.Lock multiple acquire safety issue (GH-5466

7e4cf8e

) (#5502) (cherry picked from commit d41e9e0)

bharel deleted the bpo-32734 branch Dec 23, 2019

bpo-32734: Fix asyncio.Lock multiple acquire safety issue #5466

bpo-32734: Fix asyncio.Lock multiple acquire safety issue #5466

bharel commented Jan 31, 2018 •

edited

asvetlov left a comment

asvetlov Feb 1, 2018

bharel Feb 1, 2018

asvetlov Feb 1, 2018

bharel Feb 1, 2018

bedevere-bot commented Feb 1, 2018

bedevere-bot commented Feb 1, 2018

bedevere-bot commented Feb 1, 2018

bharel commented Feb 1, 2018 •

edited

bedevere-bot commented Feb 1, 2018

1st1 Feb 2, 2018

bharel Feb 2, 2018 •

edited

1st1 Feb 2, 2018

bharel Feb 2, 2018

1st1 Feb 2, 2018

bharel Feb 2, 2018

1st1 Feb 2, 2018

bharel Feb 2, 2018 •

edited

1st1 Feb 2, 2018

bharel Feb 2, 2018

1st1 left a comment

bedevere-bot commented Feb 2, 2018

bharel Feb 2, 2018 •

edited

bharel Feb 2, 2018

bharel Feb 2, 2018

bharel Feb 2, 2018

bharel commented Feb 2, 2018

bedevere-bot commented Feb 2, 2018

asvetlov left a comment

bharel commented Feb 2, 2018

1st1 commented Feb 2, 2018

miss-islington commented Feb 2, 2018

bedevere-bot commented Feb 2, 2018

miss-islington commented Feb 2, 2018

bedevere-bot commented Feb 2, 2018

bharel commented Feb 2, 2018 •

edited

1st1 commented Feb 2, 2018


		# Make sure only one lock was taken
		self.assertEqual(lock_count, 1)

bpo-32734: Fix asyncio.Lock multiple acquire safety issue #5466

bpo-32734: Fix asyncio.Lock multiple acquire safety issue #5466

Conversation

bharel commented Jan 31, 2018 • edited

asvetlov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bedevere-bot commented Feb 1, 2018

bedevere-bot commented Feb 1, 2018

bedevere-bot commented Feb 1, 2018

bharel commented Feb 1, 2018 • edited

bedevere-bot commented Feb 1, 2018

Choose a reason for hiding this comment

bharel Feb 2, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bharel Feb 2, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

1st1 left a comment

bedevere-bot commented Feb 2, 2018

bharel Feb 2, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bharel commented Feb 2, 2018

bedevere-bot commented Feb 2, 2018

asvetlov left a comment

bharel commented Feb 2, 2018

1st1 commented Feb 2, 2018

miss-islington commented Feb 2, 2018

bedevere-bot commented Feb 2, 2018

miss-islington commented Feb 2, 2018

bedevere-bot commented Feb 2, 2018

bharel commented Feb 2, 2018 • edited

1st1 commented Feb 2, 2018

bharel commented Jan 31, 2018 •

edited

bharel commented Feb 1, 2018 •

edited

bharel Feb 2, 2018 •

edited

bharel Feb 2, 2018 •

edited

bharel Feb 2, 2018 •

edited

bharel commented Feb 2, 2018 •

edited