Skip to content
Closed
Changes from 1 commit
Commits
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
Prev Previous commit
Next Next commit
Update Lib/asyncio/runners.py
  • Loading branch information
graingert authored Jun 22, 2022
commit d11310246b2d5b40b44454a6473bd91fa5ea1443
2 changes: 1 addition & 1 deletion Lib/asyncio/runners.py
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ class Runner:

If debug is True, the event loop will be run in debug mode.
If loop_factory is passed, it is used for new event loop creation, otherwise
a ProactorEventLoop will be started on Windows and a SelectorEventLoop will
a ProactorEventLoop will be started on Windows or a SelectorEventLoop will
be started on unix
If set_policy_loop is True, the event loop in the default policy will be
set.
Comment on lines +38 to +39

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If this is classified as a bug (clearly it is causing issues), then fixing it shouldn't be blocking. By only implementing this behind an argument we are introducing a new feature which means that this will only be added to 3.12 (and not 3.11).

I would vote for not adding this argument.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think it's important to maintain the asyncio.Runner support for a high level policy-free use of asyncio, so that the policy system can be deprecated and removed

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

By adding this option we make it more difficult to deprecate the policy system because then this option will need to be deprecated as well. Users who specify this option will have more to do to migrate.

@graingert graingert Jun 27, 2022 •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

How about I remove the flag from asyncio.Runner and have callers use a wrapper that mutates the policy loop?

@contextmanager
def _with_policy_loop(loop):
    try:
        asyncio.set_event_loop(loop)
        yield
    finally:
        asyncio.set_event_loop(None)
runner = asyncio.Runner()
cmgr = _with_policy_loop(runner.get_loop())
try:
    cmgr(runner.run)(corofn())
finally:
    cmgr(runner.close)()

Users who specify this option will have more to do to migrate.

I'm proposing this set_policy_loop flag get deprecated at the same time as the policy system. I could add the flag as deprecated in this PR so users won't add it unless they already have more work to do to migrate from the policy system as a concept

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't think I understand the downside with enabling this by default. My arguments boil down to:

  • With this flag disabled, we retain the current behaviour which causes issues. I would guess that most people don't fully understand what it does and will leave it to the default, then potentially run into these issues.

  • As we've so far discussed, this flag is a fix for an issue which might soon no matter much because it is planned to be deprecated. With this option needing to be explicitly enabled, we introduce more things to deprecate. If, instead, this would be enabled by default then most would not need to change anything as it gets removed after the deprecation period.

I am not super confident in the event loop policy system, so I won't continue arguing if you don't agree with me after this comment - i'll defer to whenever a core dev takes a look at this issue.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

With this flag disabled, we retain the current behaviour which causes issues. I would guess that most people don't fully understand what it does and will leave it to the default, then potentially run into these issues.

It should be opt in for asyncio.Runner

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yep: set_policy_loop defaults to False in asyncio.Runner: def __init__(self, *, debug=None, loop_factory=None, set_policy_loop=False):

people using the old asyncio.run get the old behaviour that uses the policy loop factory and sets the policy loop. People using the new asyncio.Runner get the new behaviour which does not use the policy loop factory or set the policy loop by default

Expand Down