Skip to content
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.

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

IsolatedAsyncioTestCase and asyncio.run no-longer call asyncio.set_event_loop #93896

Closed
graingert opened this issue Jun 16, 2022 · 20 comments
Closed
Assignees
Labels
3.11 only security fixes 3.12 bugs and security fixes release-blocker topic-asyncio type-bug An unexpected behavior, bug, or error

Comments

@graingert
Copy link
Contributor

graingert commented Jun 16, 2022

in https://github.com/python/cpython/pull/31799/files#diff-1f2ae0f6c6010caf9d5f1c80cd6033a796ffe2b60554f5df84f554f4a08e622b the calls to asyncio.set_event_loop() were removed which breaks aiohttp aio-libs/aiohttp#6757 and asyncio.SafeChildWatcher

this looks like an intentional breaking change - and if it is should be documented

@graingert graingert added the type-bug An unexpected behavior, bug, or error label Jun 16, 2022
@AlexWaygood AlexWaygood added topic-asyncio 3.11 only security fixes 3.12 bugs and security fixes labels Jun 16, 2022
@graingert
Copy link
Contributor Author

graingert commented Jun 16, 2022

import sys
import asyncio


async def run_subprocess():
    proc = await asyncio.create_subprocess_shell(
        "exit 0",
        stdin=asyncio.subprocess.DEVNULL,
        stdout=asyncio.subprocess.DEVNULL,
        stderr=asyncio.subprocess.DEVNULL,
    )
    await proc.wait()
    print("success!")


async def amain():
    await asyncio.to_thread(asyncio.run, run_subprocess())


def main():
    asyncio.get_event_loop_policy().set_child_watcher(asyncio.SafeChildWatcher())
    asyncio.run(amain())


if __name__ == "__main__":
    sys.exit(main())
 graingert@superjacent  ~/projects  python3.10 demo_subprocess.py
success!
 graingert@superjacent  ~/projects  python3.11 demo_subprocess.py 
Traceback (most recent call last):
  File "/home/graingert/projects/demo_subprocess.py", line 26, in <module>
    sys.exit(main())
             ^^^^^^
  File "/home/graingert/projects/demo_subprocess.py", line 22, in main
    asyncio.run(amain())
    ^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.11/asyncio/runners.py", line 181, in run
    return runner.run(main)
           ^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.11/asyncio/runners.py", line 115, in run
    return self._loop.run_until_complete(task)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.11/asyncio/base_events.py", line 650, in run_until_complete
    return future.result()
           ^^^^^^^^^^^^^^^
  File "/home/graingert/projects/demo_subprocess.py", line 17, in amain
    await asyncio.to_thread(asyncio.run, run_subprocess())
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.11/asyncio/threads.py", line 25, in to_thread
    return await loop.run_in_executor(None, func_call)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.11/concurrent/futures/thread.py", line 58, in run
    result = self.fn(*self.args, **self.kwargs)
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.11/asyncio/runners.py", line 181, in run
    return runner.run(main)
           ^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.11/asyncio/runners.py", line 115, in run
    return self._loop.run_until_complete(task)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.11/asyncio/base_events.py", line 650, in run_until_complete
    return future.result()
           ^^^^^^^^^^^^^^^
  File "/home/graingert/projects/demo_subprocess.py", line 6, in run_subprocess
    proc = await asyncio.create_subprocess_shell(
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.11/asyncio/subprocess.py", line 205, in create_subprocess_shell
    transport, protocol = await loop.subprocess_shell(
                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.11/asyncio/base_events.py", line 1647, in subprocess_shell
    transport = await self._make_subprocess_transport(
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.11/asyncio/unix_events.py", line 204, in _make_subprocess_transport
    raise RuntimeError("asyncio.get_child_watcher() is not activated, "
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
RuntimeError: asyncio.get_child_watcher() is not activated, subprocess support is not installed.

@graingert
Copy link
Contributor Author

graingert commented Jun 16, 2022

I know there were some efforts to deprecate get/set_event_loop_policy and child watchers #82772 so I think those Deprecations should be applied before asyncio.run/IsolatedAsyncioTestCase stops calling asyncio.set_event_loop()

@graingert
Copy link
Contributor Author

graingert commented Jun 17, 2022

alternatively is there still time to deprecate the child watchers system and the policy system in favor of
asyncio.Runner(loop_factory=asyncio.ProactorEventLoop/asyncio.SelectorEventLoop/uvloop.uvloop.new_event_loop) before the 3.11 release?

that would be deprecating:

asyncio.get_event_loop() # already deprecated unless the loop is running 
asyncio.set_event_loop()  # asyncio.set_event_loop(None) should probably be exempt
asyncio.get_event_loop_policy()
asyncio.set_event_loop_policy()  # asyncio.set_event_loop_policy(None) should probably be exempt
asyncio.set_child_watcher()

asyncio.get_child_watcher() will remain and will always be asyncio.PidfdChildWatcher on old kernels and asyncio.ThreadedChildWatcher on very old kernels

asyncio.new_event_loop() will issue a DeprecationWarning if the current policy is not the default policy, and then become an alias of

if sys.platform == "win32":
    new_event_loop = ProactorEventLoop
else:
    new_event_loop = SelectorEventLoop

@graingert
Copy link
Contributor Author

this is also broken with the PidfdChildWatcher

@encukou
Copy link
Member

encukou commented Jun 27, 2022

Marking as potential release blocker.

@graingert
Copy link
Contributor Author

Marking as potential release blocker.

As this is a release blocker can you add @pablogsal as a reviewer for me?

Please, add me as a reviewer to any PR that needs to be merged to address these issues.

https://mail.python.org/archives/list/python-dev@python.org/thread/ORCYBRP432J36LXP32IDX6KLRE7Z646V/

@encukou
Copy link
Member

encukou commented Jul 4, 2022

I can. Sadly, asyncio experts seem quiet. Myself, I am out of my depth here.

@kumaraditya303
Copy link
Contributor

The PR adds a new argument to set the current event loop policy. I propose to not add a new argument and always set the loop and when in future policy setup will be deprecated, it can be reconsidered. Also I don't think we can add or remove a new argument after beta release of 3.11 but I'll defer to @pablogsal.

@pablogsal
Copy link
Member

The PR adds a new argument to set the current event loop policy.

New APIs cannot be added or modified after beta freeze. Also, existing behaviour cannot be changed unless the behaviour is fundamentally wrong.

@graingert
Copy link
Contributor Author

graingert commented Jul 4, 2022

I'm assuming that the calls to set_event_loop were removed as an intentional feature of asyncio.Runner, and ideally I'd like to keep this feature for my usage of it. However the removal of the set_event_loop calls from asyncio.run and IsolatedAsyncioTestCase appears to be to an unintended breaking change

@Bluenix2
Copy link
Contributor

Bluenix2 commented Jul 5, 2022

I am not sure I reach the same conclusion either. If I remember correctly the Runner class was extracted from some helper somewhere (may have been TaskGroup, but I am pretty sure Runner had a similar background) but I cannot find where I read this.

I did however, look at the reviews of the original PR and see that the missing set_event_loop() calls were addressed by the comments: #31799 (comment). Yes it looks to be on-purpose that they aren't called. Like you mentioned though, it does not look like the breaking changes were intended.

I'd like to keep this feature for my usage of it

Do you have a use-case where you don't want set_event_loop() to be called? I think that's what this discussion is about. What downside is there to calling set_event_loop() until it is deprecated and removed?

The Runner class is meant to abstract away working with a loop, but if we force the user to call set_event_loop() themselves then we're back at the beginning of working directly with the loop.

@graingert
Copy link
Contributor Author

Do you have a use-case where you don't want set_event_loop() to be called?

Yes, I want to be able to run multiple event loops of different flavors without interacting with global state such as the policy system. Calling set_event_loop for example can join threads in the child watcher

The Runner class is meant to abstract away working with a loop, but if we force the user to call set_event_loop() themselves then we're back at the beginning of working directly with the loop.

I'm not suggesting we force the user to do this, I'm suggesting that asyncio.run call set_event_loop for backwards compatibility purposes and asyncio.Runner is the new recommended interface, eventually phasing the policy system out entirely

@gvanrossum
Copy link
Member

I am also out of my depth here. What would be the smallest change that fixes the release blocker in 3.11, without violating the feature freeze? We should have a separate discussion about the future of the policy system.

@kumaraditya303
Copy link
Contributor

kumaraditya303 commented Jul 6, 2022

I'll create a minimal PR to unblock the release of 3.11 soon complying with feature freeze as I commented above. @gvanrossum

@gvanrossum
Copy link
Member

Go ahead with that PR, Kumar.

@kumaraditya303
Copy link
Contributor

I created #94593 which fixes this by reverting to the 3.10 and earlier behavior of setting the event loop if no loop factory was supplied. No new parameter is added so can be backport to 3.11.

@graingert
Copy link
Contributor Author

We should have a separate discussion about the future of the policy system.

see #94597

miss-islington pushed a commit to miss-islington/cpython that referenced this issue Jul 6, 2022
…ncioTestCase (pythonGH-94593)

(cherry picked from commit 14fea6b)

Co-authored-by: Kumar Aditya <59607654+kumaraditya303@users.noreply.github.com>
miss-islington added a commit that referenced this issue Jul 6, 2022
…stCase (GH-94593)

(cherry picked from commit 14fea6b)

Co-authored-by: Kumar Aditya <59607654+kumaraditya303@users.noreply.github.com>
@kumaraditya303
Copy link
Contributor

This is fixed in 3.12 (main) and 3.11 by #94593 #94608 respectively.

Thanks everyone!

@ixenion

This comment was marked as off-topic.

@gvanrossum
Copy link
Member

@ixenion Please be civil. This is not the place to get help, but you can find help for Python issues on discuss.python.org.

@python python locked as off-topic and limited conversation to collaborators Apr 24, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
3.11 only security fixes 3.12 bugs and security fixes release-blocker topic-asyncio type-bug An unexpected behavior, bug, or error
Projects
Status: Done
9 participants