I believe this change should also be implemented for the other synchronization primitives: asyncio.Event, asyncio.Condition, asyncio.Semaphore, and asyncio.BoundedSemaphore.

@aeros thanks for the review, I will fix the PR

@aeros hi I've just push the changes

@eamanu

hi I've just push the changes

Thanks. You'll need to update the tests as well.

Codecov Report

Merging #18195 into master will increase coverage by 1.07%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           master   #18195       +/-   ##
===========================================
+ Coverage   82.11%   83.19%    +1.07%     
===========================================
  Files        1954     1570      -384     
  Lines      583213   414440   -168773     
  Branches    44383    44433       +50     
===========================================
- Hits       478932   344791   -134141     
+ Misses      94652    60001    -34651     
- Partials     9629     9648       +19     

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 10355ed...44fa5f6. Read the comment docs.

@aeros I think now is ready :)

What was the reason behind removing the BoundedSemaphore deprecation warning?

Also, it would be helpful to add unit tests, to ensure the deprecation warnings are working as intended; this is done for most of the existing ones.

@aeros okas I will add tests. I remove the check on BoundedSemaphore because it inherit from Semaphore. If I let the warning, will be a double check, isn't?

@eamanu

I remove the check on BoundedSemaphore because it inherit from Semaphore. If I let the warning, will be a double check, isn't?

I hadn't previously noticed that BoundedSemaphore called super().__init__() in its __init__() method (which invokes the warning in Semaphore), I just saw the commit removing the warning from it. Good point, it can be removed from BoundedSemaphore then.

Creating a Lock object with a non-running event loop is OK as long as the loop was passed explicitly (and not obtained via a hidden call to asyncio.get_event_loop()). I suggest to change this PR to do this check:

def __init__(self, loop=None):
  if loop is None:
    loop = asyncio._get_running_loop()
    if loop is None:
      # issue warning
      loop = asyncio.get_event_loop()

(and drop the if not self._loop.is_running())

def __init__(self, loop=None):
    if loop is None:
        loop = asyncio._get_running_loop()
        if loop is None:
            # issue warning
            loop = asyncio.get_event_loop()

Ah, I see. That's likely the most explicit way to always raise when the constructor for the class is called outside of a coroutine, while also not having a second redundant deprecation warning from passing an explicit loop argument. With the current implementation in the PR, it can raise both deprecation warnings at the same time.

Out of curiosity though, would something like this work as well, or is there a situation it wouldn't appropriately cover as well as the above?:

def __init__(self, loop=None):
    if loop is None:
        loop = asyncio.get_event_loop()
        if not loop.is_running():
            # warn
            ...

Although it's not a significant concern since this is within the internals of asyncio, I typically prefer to avoid directly accessing private members of other classes when possible. IMO, it also makes the code a bit more obvious to readers since they're less likely to be as familiar with _get_running_loop().

I suspect that you may have been considering the discussion about eventually deprecating asyncio.get_event_loop(), meaning this would have to be changed (unlike your example), but I wanted to make sure that I was understanding the motivations correctly.

IMO, we need to raise the two warns (if is the case) because are two different deprecations. One of them is a warn about the loop argument deprecated where exist an explicit version where the loop argument will be removed. Meanwhile, the creation of a async object without a event loop running, is deprecated from a different version and without plan of remove it yet.

So maybe the code will be:

def __init__(self, loop=None):
  ...
  if loop is None:
    loop = asyncio._get_running_loop()
    if loop is None:
      # issue warning
      loop = asyncio.get_event_loop()
  else:
    # warn loop arg deprecated
    ... 

Also, I think that

    ...
    loop = asyncio.get_event_loop()
    if not loop.is_running():
        ...

will work, isn't?

In the other hand I agree with @aeros. I think is more redeable use asyncio.get_event_loop()
But I would like listen the @1st1 opinion :)

@aeros

Ah, I see. That's likely the most explicit way to always raise when the constructor for the class is called outside of a coroutine,

That's exactly the purpose of my solution:

1. Eventually prohibit instantiation of Lock/Semaphore/etc outside of a coroutine without an explicit loop

2. Make it possible for people to silence the warning by providing the loop explicitly without rewriting their code completely.

Out of curiosity though, would something like this work as well, or is there a situation it wouldn't appropriately cover as well as the above?:

No. _get_running_loop() is not equivalent to get_event_loop(). The former will only return a loop if it's called from a coroutine or a loop callback.

@eamanu / cc @asvetlov

IMO, we need to raise the two warns (if is the case) because are two different deprecations.

That's the thing — I don't like deprecating too much in one release. My strategy:

1. In 3.9 we deprecate instantiating a Loop/Semaphore/Future/etc outside a coroutine/loop callback without an explicit loop argument.

2. In 3.10 we can maybe deprecate the argument itself.

@1st1

No. _get_running_loop() is not equivalent to get_event_loop(). The former will only return a loop if it's called from a coroutine or a loop callback.

I think you may have misunderstood my question. I know that _get_running_loop() and get_event_loop() are not equivalent. :)

More specifically, I was referring to whether or not the following two implementation examples would cover all of the same situations:

def __init__(self, loop=None):
    if loop is None:
        loop = asyncio._get_running_loop()
        if loop is None:
            # issue warning
            loop = asyncio.get_event_loop()

def __init__(self, loop=None):
    if loop is None:
        loop = asyncio.get_event_loop()
        if not loop.is_running():
            # warn
            ...

While they use a different process to check if the event loop is running, I believe both would ultimately behave the same from a user perspective (based on my own experimentation, at least). I was wondering if there were any specific situations where the above two wouldn't behave the same (or any specific reason to use one over the other).

Yeah, both snippets are indeed similar in functionality. I'd still prefer the first one since it conditions the get_event_loop() invocation to be only for the deprecated case. IOW the first snippet reads more "clear" to me given all the subtleties of get_event_loop() behavior.

Agree with Yuri

@1st1 So, the changes looks like this, right?

That generate some errors on test, so I want be sure if I interpret correctly your idea.

+++ b/Lib/asyncio/locks.py
@@ -78,17 +78,12 @@ class Lock(_ContextManagerMixin):
         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)
-
-        if not self._loop.is_running():
-            warnings.warn("The creation of asyncio objects without a running "
-                          "event loop is deprecated as of Python 3.9.",
-                          DeprecationWarning, stacklevel=2)
+            self._loop = asyncio._get_running_loop()
+            if self._loop is None:
+                warnings.warn("The creation of asyncio objects without a running "
+                              "event loop is deprecated as of Python 3.9.",
+                              DeprecationWarning, stacklevel=2)
+                self._loop = asyncio.get_event_loop()

@aeros @asvetlov

Sorry I don't know if clear my last comments, so let just the code:

def __init__(self, *, loop=None):
        self._waiters = None
        self._locked = False
        if loop is None:
            self._loop = asyncio._get_running_loop()
            if self._loop is None:
                warnings.warn("The creation of asyncio objects without a running "
                              "event loop is deprecated as of Python 3.9.",
                              DeprecationWarning, stacklevel=2)
                self._loop = asyncio.get_event_loop()

@eamanu The logic is fine, but within the internals you should use the local module name events rather than the top-level module name asyncio to access the event loop methods. In this specific case:

def __init__(self, *, loop=None):
        self._waiters = None
        self._locked = False
        if loop is None:
            self._loop = events._get_running_loop()
            if self._loop is None:
                warnings.warn("The creation of asyncio objects without a running "
                              "event loop is deprecated as of Python 3.9.",
                              DeprecationWarning, stacklevel=2)
                self._loop = events.get_event_loop()

You can observe that the current code uses events.get_event_loop() instead of asyncio.get_event_loop() for the same reason: the top-level module name asyncio does not exist within Lib/asyncio/locks.py. To verify this, simply look at the imports at the top of the file.

Hi @aeros yes, first I was a little confusing with the @1st1 's comments and code example. But following the code example my interpretation was "deprecate instantiating a Loop/Semaphore/Future/etc outside a coroutine/loop callback without an explicit loop argument" and then mabye we can "remove the loop parameter".

But, I think yes, my interpretation is very very wrong.

So the could must be:

def __init__(self, *, loop=None):
        self._waiters = None
        self._locked = False
        if loop is None:
            self._loop = events._get_running_loop()
            if self._loop is None:
                warnings.warn("The creation of asyncio objects without a running "
                              "event loop is deprecated as of Python 3.9.",
                              DeprecationWarning, stacklevel=2)
                self._loop = events.get_event_loop()
        else:
                warnings.warn("The loop argument is deprecated since Python 3.8, "
                          "and scheduled for removal in Python 3.10.",
                          DeprecationWarning, stacklevel=2)

And go back the changes on the tests

Closing this PR following the closure of its b.p.o issue (https://bugs.python.org/issue38599)