bpo-32253: Deprecate with statement and bare await for asyncio locks (GH-4764)

* Add test for 'with (yield from lock)'
* Deprecate with statement for asyncio locks
* Document the deprecation

5 files changed, 108 insertions, 55 deletions

diff --git a/Doc/library/asyncio-sync.rst b/Doc/library/asyncio-sync.rst
index 14e3def..3e574f4 100644
--- a/Doc/library/asyncio-sync.rst
+++ b/Doc/library/asyncio-sync.rst
@@ -23,11 +23,9 @@ module (:class:`~threading.Lock`, :class:`~threading.Event`,
 :class:`~threading.BoundedSemaphore`), but it has no *timeout* parameter. The
 :func:`asyncio.wait_for` function can be used to cancel a task after a timeout.
 
-Locks
------
 
 Lock
-^^^^
+----
 
 .. class:: Lock(\*, loop=None)
 
@@ -37,8 +35,9 @@ Lock
    particular coroutine when locked.  A primitive lock is in one of two states,
    'locked' or 'unlocked'.
 
-   It is created in the unlocked state.  It has two basic methods, :meth:`acquire`
-   and :meth:`release`.  When the state is unlocked, acquire() changes the state to
+   The lock is created in the unlocked state.
+   It has two basic methods, :meth:`acquire` and :meth:`release`.
+   When the state is unlocked, acquire() changes the state to
    locked and returns immediately.  When the state is locked, acquire() blocks
    until a call to release() in another coroutine changes it to unlocked, then
    the acquire() call resets it to locked and returns.  The release() method
@@ -51,38 +50,12 @@ Lock
    resets the state to unlocked; first coroutine which is blocked in acquire()
    is being processed.
 
-   :meth:`acquire` is a coroutine and should be called with ``yield from``.
+   :meth:`acquire` is a coroutine and should be called with ``await``.
 
-   Locks also support the context management protocol.  ``(yield from lock)``
-   should be used as the context manager expression.
+   Locks support the :ref:`context management protocol <async-with-locks>`.
 
    This class is :ref:`not thread safe <asyncio-multithreading>`.
 
-   Usage::
-
-       lock = Lock()
-       ...
-       yield from lock
-       try:
-           ...
-       finally:
-           lock.release()
-
-   Context manager usage::
-
-       lock = Lock()
-       ...
-       with (yield from lock):
-           ...
-
-   Lock objects can be tested for locking state::
-
-       if not lock.locked():
-           yield from lock
-       else:
-           # lock is acquired
-           ...
-
    .. method:: locked()
 
       Return ``True`` if the lock is acquired.
@@ -110,7 +83,7 @@ Lock
 
 
 Event
-^^^^^
+-----
 
 .. class:: Event(\*, loop=None)
 
@@ -151,7 +124,7 @@ Event
 
 
 Condition
-^^^^^^^^^
+---------
 
 .. class:: Condition(lock=None, \*, loop=None)
 
@@ -166,6 +139,9 @@ Condition
    object, and it is used as the underlying lock.  Otherwise,
    a new :class:`Lock` object is created and used as the underlying lock.
 
+   Conditions support the :ref:`context management protocol
+   <async-with-locks>`.
+
    This class is :ref:`not thread safe <asyncio-multithreading>`.
 
    .. coroutinemethod:: acquire()
@@ -239,11 +215,8 @@ Condition
       This method is a :ref:`coroutine <coroutine>`.
 
 
-Semaphores
-----------
-
 Semaphore
-^^^^^^^^^
+---------
 
 .. class:: Semaphore(value=1, \*, loop=None)
 
@@ -254,12 +227,13 @@ Semaphore
    counter can never go below zero; when :meth:`acquire` finds that it is zero,
    it blocks, waiting until some other coroutine calls :meth:`release`.
 
-   Semaphores also support the context management protocol.
-
    The optional argument gives the initial value for the internal counter; it
    defaults to ``1``. If the value given is less than ``0``, :exc:`ValueError`
    is raised.
 
+   Semaphores support the :ref:`context management protocol
+   <async-with-locks>`.
+
    This class is :ref:`not thread safe <asyncio-multithreading>`.
 
    .. coroutinemethod:: acquire()
@@ -285,7 +259,7 @@ Semaphore
 
 
 BoundedSemaphore
-^^^^^^^^^^^^^^^^
+----------------
 
 .. class:: BoundedSemaphore(value=1, \*, loop=None)
 
@@ -293,3 +267,39 @@ BoundedSemaphore
 
    This raises :exc:`ValueError` in :meth:`~Semaphore.release` if it would
    increase the value above the initial value.
+
+   Bounded semapthores support the :ref:`context management
+   protocol <async-with-locks>`.
+
+   This class is :ref:`not thread safe <asyncio-multithreading>`.
+
+
+.. _async-with-locks:
+
+Using locks, conditions and semaphores in the :keyword:`async with` statement
+-----------------------------------------------------------------------------
+
+:class:`Lock`, :class:`Condition`, :class:`Semaphore`, and
+:class:`BoundedSemaphore` objects can be used in :keyword:`async with`
+statements.
+
+The :meth:`acquire` method will be called when the block is entered,
+and :meth:`release` will be called when the block is exited.  Hence,
+the following snippet::
+
+   async with lock:
+       # do something...
+
+is equivalent to::
+
+   await lock.acquire()
+   try:
+       # do something...
+   finally:
+       lock.release()
+
+.. deprecated:: 3.7
+
+   Lock acquiring using ``await lock`` or ``yield from lock`` and
+   :keyword:`with` statement (``with await lock``, ``with (yield from
+   lock)``) are deprecated.
diff --git a/Lib/asyncio/locks.py b/Lib/asyncio/locks.py
index aa6ed3e..57eb69e 100644
--- a/Lib/asyncio/locks.py
+++ b/Lib/asyncio/locks.py
@@ -3,6 +3,7 @@
 __all__ = ['Lock', 'Event', 'Condition', 'Semaphore', 'BoundedSemaphore']
 
 import collections
+import warnings
 
 from . import events
 from . import futures
@@ -63,6 +64,9 @@ class _ContextManagerMixin:
         #         <block>
         #     finally:
         #         lock.release()
+        warnings.warn("'with (yield from lock)' is deprecated "
+                      "use 'async with lock' instead",
+                      DeprecationWarning, stacklevel=2)
         yield from self.acquire()
         return _ContextManager(self)
 
@@ -71,6 +75,9 @@ class _ContextManagerMixin:
         return _ContextManager(self)
 
     def __await__(self):
+        warnings.warn("'with await lock' is deprecated "
+                      "use 'async with lock' instead",
+                      DeprecationWarning, stacklevel=2)
         # To make "with await lock" work.
         return self.__acquire_ctx().__await__()
 
diff --git a/Lib/test/test_asyncio/test_locks.py b/Lib/test/test_asyncio/test_locks.py
index c1f8d6e..f365a45 100644
--- a/Lib/test/test_asyncio/test_locks.py
+++ b/Lib/test/test_asyncio/test_locks.py
@@ -42,7 +42,8 @@ class LockTests(test_utils.TestCase):
 
         @asyncio.coroutine
         def acquire_lock():
-            yield from lock
+            with self.assertWarns(DeprecationWarning):
+                yield from lock
 
         self.loop.run_until_complete(acquire_lock())
         self.assertTrue(repr(lock).endswith('[locked]>'))
@@ -53,7 +54,8 @@ class LockTests(test_utils.TestCase):
 
         @asyncio.coroutine
         def acquire_lock():
-            return (yield from lock)
+            with self.assertWarns(DeprecationWarning):
+                return (yield from lock)
 
         res = self.loop.run_until_complete(acquire_lock())
 
@@ -63,6 +65,32 @@ class LockTests(test_utils.TestCase):
         lock.release()
         self.assertFalse(lock.locked())
 
+    def test_lock_by_with_statement(self):
+        loop = asyncio.new_event_loop()  # don't use TestLoop quirks
+        self.set_event_loop(loop)
+        primitives = [
+            asyncio.Lock(loop=loop),
+            asyncio.Condition(loop=loop),
+            asyncio.Semaphore(loop=loop),
+            asyncio.BoundedSemaphore(loop=loop),
+        ]
+
+        @asyncio.coroutine
+        def test(lock):
+            yield from asyncio.sleep(0.01, loop=loop)
+            self.assertFalse(lock.locked())
+            with self.assertWarns(DeprecationWarning):
+                with (yield from lock) as _lock:
+                    self.assertIs(_lock, None)
+                    self.assertTrue(lock.locked())
+                    yield from asyncio.sleep(0.01, loop=loop)
+                    self.assertTrue(lock.locked())
+                self.assertFalse(lock.locked())
+
+        for primitive in primitives:
+            loop.run_until_complete(test(primitive))
+            self.assertFalse(primitive.locked())
+
     def test_acquire(self):
         lock = asyncio.Lock(loop=self.loop)
         result = []
@@ -212,7 +240,8 @@ class LockTests(test_utils.TestCase):
 
         @asyncio.coroutine
         def acquire_lock():
-            return (yield from lock)
+            with self.assertWarns(DeprecationWarning):
+                return (yield from lock)
 
         with self.loop.run_until_complete(acquire_lock()):
             self.assertTrue(lock.locked())
@@ -224,7 +253,8 @@ class LockTests(test_utils.TestCase):
 
         @asyncio.coroutine
         def acquire_lock():
-            return (yield from lock)
+            with self.assertWarns(DeprecationWarning):
+                return (yield from lock)
 
         # This spells "yield from lock" outside a generator.
         cm = self.loop.run_until_complete(acquire_lock())
@@ -668,7 +698,8 @@ class ConditionTests(test_utils.TestCase):
 
         @asyncio.coroutine
         def acquire_cond():
-            return (yield from cond)
+            with self.assertWarns(DeprecationWarning):
+                return (yield from cond)
 
         with self.loop.run_until_complete(acquire_cond()):
             self.assertTrue(cond.locked())
@@ -751,7 +782,8 @@ class SemaphoreTests(test_utils.TestCase):
 
         @asyncio.coroutine
         def acquire_lock():
-            return (yield from sem)
+            with self.assertWarns(DeprecationWarning):
+                return (yield from sem)
 
         res = self.loop.run_until_complete(acquire_lock())
 
@@ -893,7 +925,8 @@ class SemaphoreTests(test_utils.TestCase):
 
         @asyncio.coroutine
         def acquire_lock():
-            return (yield from sem)
+            with self.assertWarns(DeprecationWarning):
+                return (yield from sem)
 
         with self.loop.run_until_complete(acquire_lock()):
             self.assertFalse(sem.locked())
diff --git a/Lib/test/test_asyncio/test_pep492.py b/Lib/test/test_asyncio/test_pep492.py
index 77eb7cd..4425770 100644
--- a/Lib/test/test_asyncio/test_pep492.py
+++ b/Lib/test/test_asyncio/test_pep492.py
@@ -59,12 +59,13 @@ class LockTests(BaseTest):
         async def test(lock):
             await asyncio.sleep(0.01, loop=self.loop)
             self.assertFalse(lock.locked())
-            with await lock as _lock:
-                self.assertIs(_lock, None)
-                self.assertTrue(lock.locked())
-                await asyncio.sleep(0.01, loop=self.loop)
-                self.assertTrue(lock.locked())
-            self.assertFalse(lock.locked())
+            with self.assertWarns(DeprecationWarning):
+                with await lock as _lock:
+                    self.assertIs(_lock, None)
+                    self.assertTrue(lock.locked())
+                    await asyncio.sleep(0.01, loop=self.loop)
+                    self.assertTrue(lock.locked())
+                self.assertFalse(lock.locked())
 
         for primitive in primitives:
             self.loop.run_until_complete(test(primitive))
diff --git a/Misc/NEWS.d/next/Library/2017-12-09-11-30-35.bpo-32253.TQHSYF.rst b/Misc/NEWS.d/next/Library/2017-12-09-11-30-35.bpo-32253.TQHSYF.rst
new file mode 100644
index 0000000..2916410
--- /dev/null
+++ b/Misc/NEWS.d/next/Library/2017-12-09-11-30-35.bpo-32253.TQHSYF.rst
@@ -0,0 +1,2 @@
+Deprecate ``yield from lock``, ``await lock``, ``with (yield from lock)``
+and ``with await lock`` for asyncio synchronization primitives.