10000 bpo-42392: Remove loop parameter form asyncio locks and Queue by uriyyo · Pull Request #23420 · python/cpython · GitHub
[go: up one dir, main page]
Skip to content

bpo-42392: Remove loop parameter form asyncio locks and Queue #23420

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.

Sign up for GitHub

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
merged 5 commits into from
Nov 24, 2020
Merged

bpo-42392: Remove loop parameter form asyncio locks and Queue #23420

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
152ac8a
Remove loop parameter form asyncio locks and Queue
uriyyo Nov 20, 2020
66fff45
Update Lib/asyncio/mixins.py
uriyyo Nov 20, 2020
0c764ff
Update Lib/asyncio/mixins.py
uriyyo Nov 20, 2020
a37d2bd
Remove __all__ for asyncio.mixins
uriyyo Nov 20, 2020
6b3ecfb
Update _LoopBoundedMixin class usage
uriyyo Nov 20, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
69 changes: 17 additions & 52 deletions Lib/asyncio/locks.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,9 @@
__all__ = ('Lock', 'Event', 'Condition', 'Semaphore', 'BoundedSemaphore')

import collections
import warnings

from . import events
from . import exceptions
from . import mixins


class _ContextManagerMixin:
Expand All @@ -20,7 +19,7 @@ async def __aexit__(self, exc_type, exc, tb):
self.release()


class Lock(_ContextManagerMixin):
class Lock(_ContextManagerMixin, mixins._LoopBoundedMixin):
"""Primitive lock objects.

A primitive lock is a synchronization primitive that is not owned
Expand Down Expand Up @@ -74,16 +73,9 @@ class Lock(_ContextManagerMixin):

"""

def __init__(self, *, loop=None):
def __init__(self):
self._waiters = None
self._locked = False
if loop is None:
self._loop = events.get_event_loop()
else:
self._loop = loop
warnings.warn("The loop argument is deprecated since Python 3.8, "
"and scheduled for removal in Python 3.10.",
DeprecationWarning, stacklevel=2)

def __repr__(self):
res = super().__repr__()
Expand All @@ -109,7 +101,7 @@ async def acquire(self):

if self._waiters is None:
self._waiters = collections.deque()
fut = self._loop.create_future()
fut = self._get_loop().create_future()
self._waiters.append(fut)

# Finally block should be called before the CancelledError
Expand Down Expand Up @@ -161,7 +153,7 @@ def _wake_up_first(self):
fut.set_result(True)


class Event:
class Event(mixins._LoopBoundedMixin):
"""Asynchronous equivalent to threading.Event.

Class implementing event objects. An event manages a flag that can be set
Expand All @@ -170,16 +162,9 @@ class Event:
false.
"""

def __init__(self, *, loop=None):
def __init__(self):
self._waiters = collections.deque()
self._value = False
if loop is None:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd actually keep the loop=None parameter and add something like this:

if loop is not None:
    raise TypeError(
        f'passing explicit "loop" argument to {type(self).__name__}() '
        f'is no longer necessary')

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought that we should fully remove loop parameter because it deprecated.

Maybe it's better to fully remove loop as for me it a lit bit confusing to have a parameter that raises an exception when it set, it looks the same as we won't have loop parameter when it set we will receive an error that init method doesn't accept loop but it has a drawback - exception message won't be such explicit.

What is your opinion?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could do marker=object() and then use the loop=marker default to make it better. But I'm also not 100% about my own suggestion here :) @asvetlov thoughts?

Copy link
Contributor
@aeros aeros Nov 24, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm +1 for raising an explicit error and using the sentinel approach to check if the user passed a loop arg. Especially with how widespread the loop parameter is used throughout existing asyncio code, it's going to be a much smoother transition to have an error message that tells users precisely what they did incorrect rather than the parameter just disappearing.

(Also keeping in mind the unfortunate reality that not everyone tests with deprecation warnings on, or might not have gotten around to removing loop throughout their own code since the warnings started in 3.8)

As for when the parameter should just be removed entirely without an explicit error message, I think doing that in 3.11 or 3.12 would be fine.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now I get it, sorry that didn't understand it the first time.

Give an explicit error will be better for debugging and will make the transition much easier.
I will update #23499 to use this approach regarding the loop parameter.

Copy link
Contributor
@aeros aeros Nov 24, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although I would make the message a little bit more direct and technically specific:

if loop is not None:
    raise TypeError(
        f'As of 3.10, the *loop* parameter was removed from {type(self).__name__}() ')
        f'since it is no longer necessary')

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

loop=marker looks a little more protective but, I think, the whole idea is a slight overcomplication.
For me, "removal" means exactly removal but I can live with the proposed change.

self._loop = events.get_event_loop()
else:
self._loop = loop
warnings.warn("The loop argument is deprecated since Python 3.8, "
"and scheduled for removal in Python 3.10.",
DeprecationWarning, stacklevel=2)

def __repr__(self):
res = super().__repr__()
Expand Down Expand Up @@ -220,7 +205,7 @@ async def wait(self):
if self._value:
return True

fut = self._loop.create_future()
fut = self._get_loop().create_future()
self._waiters.append(fut)
try:
await fut
Expand All @@ -229,7 +214,7 @@ async def wait(self):
self._waiters.remove(fut)


class Condition(_ContextManagerMixin):
class Condition(_ContextManagerMixin, mixins._LoopBoundedMixin):
"""Asynchronous equivalent to threading.Condition.

This class implements condition variable objects. A condition variable
Expand All @@ -239,18 +224,10 @@ class Condition(_ContextManagerMixin):
A new Lock object is created and used as the underlying lock.
"""

def __init__(self, lock=None, *, loop=None):
if loop is None:
self._loop = events.get_event_loop()
else:
self._loop = loop
warnings.warn("The loop argument is deprecated since Python 3.8, "
"and scheduled for removal in Python 3.10.",
DeprecationWarning, stacklevel=2)

def __init__(self, lock=None):
if lock is None:
lock = Lock(loop=loop)
elif lock._loop is not self._loop:
lock = Lock()
elif lock._loop is not self._get_loop():
raise ValueError("loop argument must agree with lock")

self._lock = lock
Expand Down Expand Up @@ -284,7 +261,7 @@ async def wait(self):

self.release()
try:
fut = self._loop.create_future()
fut = self._get_loop().create_future()
self._waiters.append(fut)
try:
await fut
Expand Down Expand Up @@ -351,7 +328,7 @@ def notify_all(self):
self.notify(len(self._waiters))


class Semaphore(_ContextManagerMixin):
class Semaphore(_ContextManagerMixin, mixins._LoopBoundedMixin):
"""A Semaphore implementation.

A semaphore manages an internal counter which is decremented by each
Expand All @@ -366,18 +343,11 @@ class Semaphore(_ContextManagerMixin):
ValueError is raised.
"""

def __init__(self, value=1, *, loop=None):
def __init__(self, value=1):
if value < 0:
raise ValueError("Semaphore initial value must be >= 0")
self._value = value
self._waiters = collections.deque()
if loop is None:
self._loop = events.get_event_loop()
else:
self._loop = loop
warnings.warn("The loop argument is deprecated since Python 3.8, "
"and scheduled for removal in Python 3.10.",
DeprecationWarning, stacklevel=2)

def __repr__(self):
res = super().__repr__()
Expand Down Expand Up @@ -407,7 +377,7 @@ async def acquire(self):
True.
"""
while self._value <= 0:
fut = self._loop.create_future()
fut = self._get_loop().create_future()
self._waiters.append(fut)
try:
await fut
Expand Down Expand Up @@ -436,14 +406,9 @@ class BoundedSemaphore(Semaphore):
above the initial value.
"""

def __init__(self, value=1, *, loop=None):
if loop:
warnings.warn("The loop argument is deprecated since Python 3.8, "
"and scheduled for removal in Python 3.10.",
DeprecationWarning, stacklevel=2)

def __init__(self, value=1):
self._bound_value = value
super().__init__(value, loop=loop)
super().__init__(value)

def release(self):
if self._value >= self._bound_value:
Expand Down
21 changes: 21 additions & 0 deletions Lib/asyncio/mixins.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
"""Event loop mixins."""

import threading
from . import events

_global_lock = threading.Lock()


class _LoopBoundedMixin:
_loop = None

def _get_loop(self):
loop = events._get_running_loop()

if self._loop is None:
with _global_lock:
if self._loop is None:
self._loop = loop
if loop is not self._loop:
raise RuntimeError(f'{type(self).__name__} have already bounded to another loop')
return loop
20 changes: 6 additions & 14 deletions Lib/asyncio/queues.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,9 @@

import collections
import heapq
import warnings

from . import events
from . import locks
from . import mixins


class QueueEmpty(Exception):
Expand All @@ -18,7 +17,7 @@ class QueueFull(Exception):
pass


class Queue:
class Queue(mixins._LoopBoundedMixin):
"""A queue, useful for coordinating producer and consumer coroutines.

If maxsize is less than or equal to zero, the queue size is infinite. If it
Expand All @@ -30,22 +29,15 @@ class Queue:
interrupted between calling qsize() and doing an operation on the Queue.
"""

def __init__(self, maxsize=0, *, loop=None):
if loop is None:
self._loop = events.get_event_loop()
else:
self._loop = loop
warnings.warn("The loop argument is deprecated since Python 3.8, "
"and scheduled for removal in Python 3.10.",
DeprecationWarning, stacklevel=2)
def __init__(self, maxsize=0):
self._maxsize = maxsize

# Futures.
self._getters = collections.deque()
# Futures.
self._putters = collections.deque()
self._unfinished_tasks = 0
self._finished = locks.Event(loop=loop)
self._finished = locks.Event()
self._finished.set()
self._init(maxsize)

Expand Down Expand Up @@ -122,7 +114,7 @@ async def put(self, item):
slot is available before adding item.
"""
while self.full():
putter = self._loop.create_future()
putter = self._get_loop().create_future()
self._putters.append(putter)
try:
await putter
Expand Down Expand Up @@ -160,7 +152,7 @@ async def get(self):
If queue is empty, wait until an item is available.
"""
while self.empty():
getter = self._loop.create_future()
getter = self._get_loop().create_future()
self._getters.append(getter)
try:
await getter
Expand Down
2 changes: 1 addition & 1 deletion Lib/asyncio/tasks.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -578,7 +578,7 @@ def as_completed(fs, *, loop=None, timeout=None):
raise TypeError(f"expect an iterable of futures, not {type(fs).__name__}")

from .queues import Queue # Import here to avoid circular import problem.
done = Queue(loop=loop)
done = Queue()

if loop is None:
loop = events.get_event_loop()
Expand Down
Loading
0