bpo-38599: Deprecate creation of asyncio object when the loop is not running #18195

eamanu · 2020-01-26T23:49:32Z

Add Deprecation Warning when Queue and Locks are created and
the loop is not running.

https://bugs.python.org/issue38599

Add Deprecation Warning when Queue and Locks are created and the loop is not running.

Lib/asyncio/locks.py

aeros · 2020-01-27T05:44:32Z

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

eamanu · 2020-01-27T11:39:05Z

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

@aeros

Add @aeros comments

eamanu · 2020-02-05T22:27:20Z

@aeros hi I've just push the changes

aeros · 2020-02-06T06:25:23Z

@eamanu

hi I've just push the changes

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

codecov · 2020-02-10T02:19:57Z

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

Impacted Files	Coverage Δ
Lib/test/test_distutils.py	`66.66% <0.00%> (-4.77%)`	⬇️
Lib/concurrent/futures/thread.py	`64.70% <0.00%> (-3.52%)`	⬇️
Lib/concurrent/futures/process.py	`55.72% <0.00%> (-1.47%)`	⬇️
Lib/test/test_largefile.py	`90.54% <0.00%> (-1.04%)`	⬇️
Lib/test/test_fractions.py	`96.70% <0.00%> (-0.34%)`	⬇️
Lib/random.py	`88.12% <0.00%> (-0.28%)`	⬇️
Lib/test/test_buffer.py	`95.14% <0.00%> (-0.21%)`	⬇️
Lib/test/test_zipfile.py	`98.01% <0.00%> (-0.20%)`	⬇️
Lib/test/test_asyncore.py	`93.62% <0.00%> (-0.18%)`	⬇️
Lib/test/test_asyncio/test_pep492.py	`93.75% <0.00%> (-0.17%)`	⬇️
... and 430 more

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.

eamanu · 2020-02-10T02:32:19Z

@aeros I think now is ready :)

aeros · 2020-02-10T07:28:49Z

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.

eamanu · 2020-02-10T10:50:47Z

@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?

aeros · 2020-02-11T08:47:34Z

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

aeros

Mostly looks good now, just a few points to address in the tests.

Note: The suggestions below apply to all of the tests, but I only commented on one of them to avoid repeating the same message.

Lib/test/test_asyncio/test_locks.py

aeros

LGTM, thanks for making the recommended changes @eamanu!

As a minor note, I think a brief docstring beneath the new class with something along the lines of Tests the deprecation of creating asyncio objects outside of a running event loop. would be helpful to document the purpose of the tests (since it may not be immediately clear what the deprecation is testing). But I don't think that's 100% necessary, and the PR could likely be merged without it.

Also for formatting, there's typically a space between the class declaration and the first method (which is followed for the above test class). That should be a quick fix. (:

Lib/test/test_asyncio/test_locks.py

Lib/test/test_asyncio/test_queues.py

Misc/NEWS.d/next/Library/2020-01-26-20-54-04.bpo-38599.0PObOD.rst

1st1 · 2020-02-26T00:11:06Z

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())

aeros · 2020-02-26T09:18:00Z

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.

eamanu · 2020-02-27T03:26:32Z

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 :)

1st1 · 2020-02-27T04:41:35Z

@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:

Eventually prohibit instantiation of Lock/Semaphore/etc outside of a coroutine without an explicit loop
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:

In 3.9 we deprecate instantiating a Loop/Semaphore/Future/etc outside a coroutine/loop callback without an explicit loop argument.
In 3.10 we can maybe deprecate the argument itself.

aeros · 2020-02-27T11:11:07Z

@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).

1st1 · 2020-02-27T18:21:04Z

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.

asvetlov · 2020-02-27T21:04:44Z

Agree with Yuri

eamanu · 2020-03-05T13:28:00Z

@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

eamanu · 2020-03-05T13:31:18Z

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()

aeros · 2020-03-06T01:09:12Z

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

aeros

Also, a bit of a bikeshed on the warning message: I'm thinking that "The creation of asyncio objects outside of a running event loop is deprecated..." will be more immediately clear to users and technically correct compared to "The creation of asyncio objects without a running event loop is deprecated...".

(I suggested the current version previously, but after having some additional time to think about it, I think the above is more clear)

Lib/asyncio/locks.py

Co-Authored-By: Kyle Stanley <aeros167@gmail.com>

aeros

@eamanu I don't think the intention from @1st1 was to remove the existing deprecation warnings for the loop argument that were added in 3.8:

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

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

In 3.10 we can maybe deprecate the argument itself.

Yury can correct me if I misunderstood what he meant, but I believe he was referring to deprecating the loop argument in all situations in 3.10. With his plan, explicitly passing the event loop would continue to raise a deprecation warning in 3.9 if the user attempted to do so within a running event loop (such as within an async def function/method), as it already does in 3.8. Then in 3.10, we could consider also deprecating the loop argument when it's passed outside of a running event loop.

But regardless of what Yury meant, I personally disagree with entirely removing the existing deprecation warnings that were added in 3.8, as the PR currently is doing:

aeros · 2020-03-16T04:13:47Z

Lib/asyncio/locks.py

-            warnings.warn("The loop argument is deprecated since Python 3.8, "
-                          "and scheduled for removal in Python 3.10.",
-                          DeprecationWarning, stacklevel=2)


It would be very confusing to users to add a deprecation warning in 3.8, and then remove the warning from certain asyncio objects entirely in 3.9. That doesn't make much sense to me.

(also applies to the other warning removals)

eamanu · 2020-03-18T02:02:39Z

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

iritkatriel · 2021-05-29T13:06:54Z

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

Deprecate creation of asyncio object when the loop is not running

2941d1e

Add Deprecation Warning when Queue and Locks are created and the loop is not running.

eamanu requested review from 1st1, asvetlov and rhettinger as code owners January 26, 2020 23:49

the-knights-who-say-ni added the CLA signed label Jan 26, 2020

bedevere-bot added the awaiting review label Jan 26, 2020

Add NEWS

7958de9

aeros reviewed Jan 27, 2020

View reviewed changes

Lib/asyncio/locks.py Outdated Show resolved Hide resolved

Fix Deprecation Warning message.

976a808

Add @aeros comments

Remove change on BoundedSemaphore

44fa5f6

add test

e486fe4

eamanu force-pushed the fix-38599 branch from 10a0eef to e486fe4 Compare February 11, 2020 03:07

Merge branch 'master' into fix-38599

bae5755

aeros suggested changes Feb 11, 2020

View reviewed changes

Lib/test/test_asyncio/test_locks.py Outdated Show resolved Hide resolved

Lib/test/test_asyncio/test_locks.py Outdated Show resolved Hide resolved

Lib/test/test_asyncio/test_locks.py Outdated Show resolved Hide resolved

bedevere-bot added awaiting core review and removed awaiting review labels Feb 11, 2020

aeros reviewed Feb 11, 2020

View reviewed changes

Lib/test/test_asyncio/test_locks.py Outdated Show resolved Hide resolved

Fix tests

18c4ddf

aeros approved these changes Feb 16, 2020

View reviewed changes

Lib/test/test_asyncio/test_locks.py Show resolved Hide resolved

Lib/test/test_asyncio/test_queues.py Outdated Show resolved Hide resolved

aeros reviewed Feb 16, 2020

View reviewed changes

Misc/NEWS.d/next/Library/2020-01-26-20-54-04.bpo-38599.0PObOD.rst Outdated Show resolved Hide resolved

Fix PR according to @aeros comments

dc78f3a

aeros reviewed Mar 6, 2020

View reviewed changes

Fix according to review comments

1bb46a8

aeros reviewed Mar 15, 2020

View reviewed changes

Lib/asyncio/locks.py Outdated Show resolved Hide resolved

eamanu and others added 5 commits March 15, 2020 23:15

remove assertWarns on where is not applied

0ca9f61

Update Lib/asyncio/locks.py

c4afc8f

Co-Authored-By: Kyle Stanley <aeros167@gmail.com>

fix grammar error

ec3a6aa

Remove assertWarns not applied on queues test

dd6dec3

Merge branch 'fix-38599' of github.com:eamanu/cpython into fix-38599

c5d332a

aeros suggested changes Mar 16, 2020

View reviewed changes

iritkatriel closed this May 29, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpo-38599: Deprecate creation of asyncio object when the loop is not running #18195

bpo-38599: Deprecate creation of asyncio object when the loop is not running #18195

eamanu commented Jan 26, 2020 •

edited by bedevere-bot

Loading

aeros commented Jan 27, 2020

eamanu commented Jan 27, 2020

eamanu commented Feb 5, 2020

aeros commented Feb 6, 2020

codecov bot commented Feb 10, 2020

eamanu commented Feb 10, 2020

aeros commented Feb 10, 2020 •

edited

Loading

eamanu commented Feb 10, 2020

aeros commented Feb 11, 2020

aeros left a comment

aeros left a comment

1st1 commented Feb 26, 2020

aeros commented Feb 26, 2020 •

edited

Loading

eamanu commented Feb 27, 2020 •

edited

Loading

1st1 commented Feb 27, 2020

aeros commented Feb 27, 2020 •

edited

Loading

1st1 commented Feb 27, 2020

asvetlov commented Feb 27, 2020

eamanu commented Mar 5, 2020

eamanu commented Mar 5, 2020

aeros commented Mar 6, 2020 •

edited

Loading

aeros left a comment

aeros left a comment •

edited

Loading

aeros Mar 16, 2020 •

edited

Loading

eamanu commented Mar 18, 2020

iritkatriel commented May 29, 2021

bpo-38599: Deprecate creation of asyncio object when the loop is not running #18195

bpo-38599: Deprecate creation of asyncio object when the loop is not running #18195

Conversation

eamanu commented Jan 26, 2020 • edited by bedevere-bot Loading

aeros commented Jan 27, 2020

eamanu commented Jan 27, 2020

eamanu commented Feb 5, 2020

aeros commented Feb 6, 2020

codecov bot commented Feb 10, 2020

Codecov Report

eamanu commented Feb 10, 2020

aeros commented Feb 10, 2020 • edited Loading

eamanu commented Feb 10, 2020

aeros commented Feb 11, 2020

aeros left a comment

Choose a reason for hiding this comment

aeros left a comment

Choose a reason for hiding this comment

1st1 commented Feb 26, 2020

aeros commented Feb 26, 2020 • edited Loading

eamanu commented Feb 27, 2020 • edited Loading

1st1 commented Feb 27, 2020

aeros commented Feb 27, 2020 • edited Loading

1st1 commented Feb 27, 2020

asvetlov commented Feb 27, 2020

eamanu commented Mar 5, 2020

eamanu commented Mar 5, 2020

aeros commented Mar 6, 2020 • edited Loading

aeros left a comment

Choose a reason for hiding this comment

aeros left a comment • edited Loading

Choose a reason for hiding this comment

aeros Mar 16, 2020 • edited Loading

Choose a reason for hiding this comment

eamanu commented Mar 18, 2020

iritkatriel commented May 29, 2021

eamanu commented Jan 26, 2020 •

edited by bedevere-bot

Loading

aeros commented Feb 10, 2020 •

edited

Loading

aeros commented Feb 26, 2020 •

edited

Loading

eamanu commented Feb 27, 2020 •

edited

Loading

aeros commented Feb 27, 2020 •

edited

Loading

aeros commented Mar 6, 2020 •

edited

Loading

aeros left a comment •

edited

Loading

aeros Mar 16, 2020 •

edited

Loading