Workaround for asyncio.get_event_loop() deprecation #355

pepoluan · 2022-12-28T09:40:30Z

What do these changes do?

Implement workaround due to the deprecation in behavior of asyncio.get_event_loop() (which will outright result in error with Python 3.12)

This change of behavior first appeared in Python 3.10: https://docs.python.org/3.10/library/asyncio-eventloop.html#asyncio.get_event_loop

And there has been some discussion on this, among others:

Are there changes in behavior for the user?

Should be minimal.

If user creates their own event loop and feeds that to aiosmtpd, then nothing will be changed
If user does not create their own event loop and let aiosmtpd generates one for them, then the behavior totally depends on how get_event_loop_policy() was in effect at time of instantiation.
- This might or might not be a problem. For asyncio, the behaviour should be similar. I am not sure about the behavior of other implementations (e.g., uvloop)

Related issue number

Closes #353

Checklist

I think the code is well written
Unit tests for the changes exist
tox testenvs have been executed in the following environments:
- Windows (10): py311-{nocov,cov}
Documentation reflects the changes

codecov · 2022-12-28T09:55:02Z

Codecov Report

Merging #355 (0271497) into master (e4bcd3f) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master      #355   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            7         7           
  Lines         1696      1706   +10     
  Branches       310       310           
=========================================
+ Hits          1696      1706   +10

Impacted Files	Coverage Δ
aiosmtpd/__init__.py	`100.00% <100.00%> (ø)`
aiosmtpd/handlers.py	`100.00% <100.00%> (ø)`
aiosmtpd/main.py	`100.00% <100.00%> (ø)`
aiosmtpd/smtp.py	`100.00% <100.00%> (ø)`

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

pepoluan · 2022-12-28T12:30:49Z

aiosmtpd/__init__.py

+        warnings.simplefilter("ignore")
+        try:
+            loop = asyncio.get_event_loop()
+        except (DeprecationWarning, RuntimeError):  # pragma: no cover


When 3.12 comes out we might have to revisit this and change the caught exception in accordance to the actual exception that will be raised.

strongholdmedia · 2022-12-28T17:04:15Z

Is there any sensible write-up as per why (and perhaps when) this policy stuff was introduced with asyncio?

pepoluan · 2022-12-28T19:01:28Z

Is there any sensible write-up as per why (and perhaps when) this policy stuff was introduced with asyncio?

The deprecation notice first appeared with Python 3.10: https://docs.python.org/3.10/library/asyncio-eventloop.html#asyncio.get_event_loop

There's a discussion about this here: python/cpython#83710

What's being discussed is over my head atm, though. So I'm just taking the 'easy way out' trying to ensure compat by implementing a workaround. And one ugly warning suppression.

Edit: More dicussion here: python/cpython#93453

pepoluan · 2022-12-28T19:15:59Z

I hope @asvetlov can chime in here. I'm not sure where things are going, and I'd like this PR to be both backward- and forward-compatible with Python >= 3.7

The current changes indeed pass green when fed through the GHCI testing fleet, but it's still making me uneasy.

waynew · 2022-12-28T19:18:47Z

I've taken a quick glance at this and based on the discussion I'm pretty sure that we need to make a decision within aiosmtpd:

Do we want to take responsibility for the created event loop?

It's looking like historically that's caused problems for folks generally. I think in our case we have two different scenarios:

aiosmtpd is the top-level app/process and should go ahead and create the event loop. Based on what I'm seeing on that thread, that should happen via .run().
aiosmtpd is used as a library (like I do in orouboros), and the authors should manage the loop.

What that looks like, I think, is that we should use loop or asyncio.get_running_loop().

Comments, questions, rebuttals?

pepoluan · 2022-12-28T19:31:54Z

I've taken a quick glance at this and based on the discussion I'm pretty sure that we need to make a decision within aiosmtpd:

Do we want to take responsibility for the created event loop?

It's looking like historically that's caused problems for folks generally. I think in our case we have two different scenarios:
1. aiosmtpd is the top-level app/process and should go ahead and create the event loop. Based on what I'm seeing on that thread, that should happen via `.run()`.

2. aiosmtpd is used as a library (like I do in [orouboros](https://github.com/waynew/orouboros)), and the authors should manage the loop.

I'm all for treating aiosmtpd as a pure library, and letting users of aiosmtpd to provide the loop.

However that will introduce some possibly-breaking changes, especially for users that currently uses aiosmtpd as-is, relying on one of the Controllers to spin up an event loop. Like for instance one of the reasons for creation of UnthreadedController which allows user to spin up a different process and let UnthreadedController sets things up in that child process with minimal prepping.

So if we want to to go "pure library" route -- which for the record I'm all for it -- that change will need to wait until at the very least 1.5.0 ... or even 1.6.0 or 2.0.0

This PR is just kind of a band-aid over the underlying problem. At the very least it's meant to provide compat for 3.7 <= Python <= 3.12

waynew · 2022-12-28T20:18:24Z

It looks like get_running_loop exists in 3.7 https://docs.python.org/3/library/asyncio-eventloop.html#asyncio.get_running_loop

We should be able to use that - and if we want to retain compatibility just catch RuntimeError and start our own event loop.

For the record, I'm entirely OK with having entrypoints (e.g. http.server) that will actually spin up loops, we just need to do that in a responsible way.

pepoluan · 2022-12-29T04:36:10Z

It looks like get_running_loop exists in 3.7 https://docs.python.org/3/library/asyncio-eventloop.html#asyncio.get_running_loop

We should be able to use that - and if we want to retain compatibility just catch RuntimeError and start our own event loop.

There's a catch though:

This function can only be called from a coroutine or a callback.

Some calls to get_event_loop() took place in the main thread, and there get_running_loop() doesn't work. I forgot exactly what happened when I tried that, but tests got wonky.

That import was used only to get the current event loop. Now we handle that in __init__.py

Also simplify the line because we have a helper func now

Because at the point the expected behavior is not yet happening.

Because apparently in Python<3.12, even though get_event_loop() raises a warning, it still returns existing event loop. The behavior is expected to change on Python>=3.12, so let's cut off at that point.

pepoluan · 2022-12-29T06:14:19Z

aiosmtpd/__init__.py

+        try:
+            loop = asyncio.get_event_loop()
+        except (DeprecationWarning, RuntimeError):  # pragma: py-lt-310
+            if loop is None:  # pragma: py-lt-312


On Python 3.11 (and I believe also 3.10), though get_event_loop() raises a warning when there's no existing event loop, it does indeed return a newly-created event loop. This defensive check prevents (re-)creation of a new event loop in that situation.

On Python 3.12 the behavior likely changes, i.e., no implicit creation of event loop inside get_event_loop(). In that situation, loop will remain None and only in that case will we create a new event loop.

That is why this particular check has a # pragma: py-lt-312 mark there, because the branch will never be taken for Python<3.12

pepoluan · 2022-12-29T06:21:10Z

Aaaaand it seems that the new Warning behavior is getting reverted?

python/cpython#100412

Nevertheless I still want to merge this changes to Cover-Our-Posteriors, so to speak.

Edit: If indeed removal gets rescheduled as Guido informed here we will need to revisit the # pragma: markers, but the logic itself I think is quite sound and adaptable (doesn't try to do things based on Python version)

waynew · 2022-12-29T15:35:44Z

Well, they're just deferred, and will eventually fail. I only glanced at this, if you'd like me to do a more thorough review I can, otherwise I'm OK with this approach for now

pepoluan · 2022-12-29T19:00:08Z

Yeah, this PR basically just put a wrapper around asyncio.get_event_loop() to maintain the behavior of the function up to Python 3.9. So just a band-aid to the deeper problems.

I'd say for long term, just like you mentioned, we need to take a step back and implement asyncio more properly. And in the process probably we can also handle Issue #334 by using the 'more modern' way of using asyncio.

pepoluan · 2022-12-30T10:59:40Z

This PR is not really 'urgent', though I've heard some complaints about the DeprecationWarning.

I'll let this simmer a bit for several more days just to ensure that I haven't overlooked something...

waynew

I didn't dig to check for places this should have been used or anything, but generally I do like this, and don't see any problems.

waynew · 2023-01-02T00:56:26Z

aiosmtpd/__init__.py

+            if loop is None:  # pragma: py-lt-312
+                loop = asyncio.new_event_loop()
+                asyncio.set_event_loop(loop)
+    assert isinstance(loop, asyncio.AbstractEventLoop)


comment FWIW this is probably less useful, since assertions can be disabled. And everything will go sideways shortly after this function gets called anyway 😂

It's mostly for type checking, not expected to do anything useful 😂

pepoluan added this to the 1.4.4 milestone Dec 28, 2022

pepoluan added compatibility tech-debt Things that needs to be tidied up to avoid being bitten in the future... labels Dec 28, 2022

pepoluan force-pushed the get-event-loop-deprecation branch 2 times, most recently from 8f1fd49 to 718c45a Compare December 28, 2022 11:55

pepoluan requested review from warsaw and waynew December 28, 2022 12:28

pepoluan self-assigned this Dec 28, 2022

pepoluan requested a review from asvetlov December 28, 2022 12:29

pepoluan commented Dec 28, 2022

View reviewed changes

pepoluan mentioned this pull request Dec 28, 2022

Fix SMTPS / implicit TLS #292

Open

pepoluan added 7 commits December 29, 2022 12:43

Workaround for asyncio.get_event_loop() deprecation

cf1f2ae

Remove unused import in main.py

25cbb08

That import was used only to get the current event loop. Now we handle that in __init__.py

BUGFIX: if loop is not None then use that

c950c78

Also simplify the line because we have a helper func now

Add pragma: no cover

528e46e

Because at the point the expected behavior is not yet happening.

Update NEWS + bump to 1.4.4a1

875679c

Don't ignore the warning, rather promote it into an error

a9c5fc8

Some test cases to ensure the get event func works as intended

502852a

pepoluan force-pushed the get-event-loop-deprecation branch from 718c45a to 502852a Compare December 29, 2022 05:46

pepoluan added 3 commits December 29, 2022 12:56

Skip coverage only on Python<3.10

667464a

Create new event loop only on failure

f8128e7

Because apparently in Python<3.12, even though get_event_loop() raises a warning, it still returns existing event loop. The behavior is expected to change on Python>=3.12, so let's cut off at that point.

More stringent testing for _get_or_new_eventloop

462b3c0

pepoluan commented Dec 29, 2022

View reviewed changes

FIX: Type hint for fixture should be Generator[]

0271497

waynew approved these changes Jan 2, 2023

View reviewed changes

pepoluan merged commit ba2b0c3 into aio-libs:master Jan 13, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Workaround for asyncio.get_event_loop() deprecation #355

Workaround for asyncio.get_event_loop() deprecation #355

pepoluan commented Dec 28, 2022 •

edited

Loading

codecov bot commented Dec 28, 2022 •

edited

Loading

pepoluan Dec 28, 2022

strongholdmedia commented Dec 28, 2022

pepoluan commented Dec 28, 2022 •

edited

Loading

pepoluan commented Dec 28, 2022

waynew commented Dec 28, 2022

pepoluan commented Dec 28, 2022

waynew commented Dec 28, 2022

pepoluan commented Dec 29, 2022

pepoluan Dec 29, 2022 •

edited

Loading

pepoluan commented Dec 29, 2022 •

edited

Loading

waynew commented Dec 29, 2022

pepoluan commented Dec 29, 2022

pepoluan commented Dec 30, 2022

waynew left a comment

waynew Jan 2, 2023

pepoluan Jan 13, 2023

Workaround for asyncio.get_event_loop() deprecation #355

Workaround for asyncio.get_event_loop() deprecation #355

Conversation

pepoluan commented Dec 28, 2022 • edited Loading

What do these changes do?

Are there changes in behavior for the user?

Related issue number

Checklist

codecov bot commented Dec 28, 2022 • edited Loading

Codecov Report

pepoluan Dec 28, 2022

Choose a reason for hiding this comment

strongholdmedia commented Dec 28, 2022

pepoluan commented Dec 28, 2022 • edited Loading

pepoluan commented Dec 28, 2022

waynew commented Dec 28, 2022

pepoluan commented Dec 28, 2022

waynew commented Dec 28, 2022

pepoluan commented Dec 29, 2022

pepoluan Dec 29, 2022 • edited Loading

Choose a reason for hiding this comment

pepoluan commented Dec 29, 2022 • edited Loading

waynew commented Dec 29, 2022

pepoluan commented Dec 29, 2022

pepoluan commented Dec 30, 2022

waynew left a comment

Choose a reason for hiding this comment

waynew Jan 2, 2023

Choose a reason for hiding this comment

pepoluan Jan 13, 2023

Choose a reason for hiding this comment

pepoluan commented Dec 28, 2022 •

edited

Loading

codecov bot commented Dec 28, 2022 •

edited

Loading

pepoluan commented Dec 28, 2022 •

edited

Loading

pepoluan Dec 29, 2022 •

edited

Loading

pepoluan commented Dec 29, 2022 •

edited

Loading