10000 bpo-33562: Check for asyncio global setting changes in tests by brettcannon · Pull Request #6958 · python/cpython · GitHub
[go: up one dir, main page]
Skip to content

bpo-33562: Check for asyncio global setting changes in tests #6958

New issue 8000

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 7 commits into from
Closed

bpo-33562: Check for asyncio global setting changes in tests #6958

wants to merge 7 commits into from

Conversation

brettcannon
Copy link
Member
@brettcannon brettcannon commented May 17, 2018 •
edited
Loading

Tests that need to be updated:

  1. test_asyncio

@brettcannon brettcannon added tests Tests in the Lib/test dir DO-NOT-MERGE labels May 17, 2018
@brettcannon brettcannon requested a review from 1st1 May 17, 2018 21:03
@brettcannon
Copy link
Member Author

I think at this point I don't know quite how to fix test_asyncio, @1st1 (it hangs indefinitely on test_running_loop_within_a_loop).

@1st1
Copy link
Member
1st1 commented May 18, 2018

@asvetlov Andrew, please take a look if you have time.

@willingc
Copy link
Contributor

@brettcannon I've looked at the Mac VSTS failures for test_async.

FWIW we have seen flaky test on JupyterHub when using mock with async. Usually it is due to an intermittent timing issue or race condition.

https://github.com/python/cpython/blob/master/Lib/asyncio/futures.py#L37

I think that future.result is not thread-safe and may be part of the flaky tests. loop.call_soon may need to be called somewhere.

On another note, we have found that increasing the async timeout when running tests on the mac also helps.

Example test failure:

2018-05-21T05:04:32.7143170Z ERROR: test_sock_sendfile_no_fallback (test.test_asyncio.test_base_events.BaseLoopSockSendfileTests)
2018-05-21T05:04:32.7160310Z ----------------------------------------------------------------------
2018-05-21T05:04:32.7176950Z Traceback (most recent call last):
2018-05-21T05:04:32.7192580Z   File "/Users/vsts/agent/2.133.3/work/1/s/Lib/test/test_asyncio/test_base_events.py", line 1892, in test_sock_sendfile_no_fallback
2018-05-21T05:04:32.7208330Z     sock, proto = self.prepare()
2018-05-21T05:04:32.7229220Z   File "/Users/vsts/agent/2.133.3/work/1/s/Lib/test/test_asyncio/test_base_events.py", line 1866, in prepare
2018-05-21T05:04:32.7250460Z     self.run_loop(self.loop.sock_connect(sock, (support.HOST, port)))
2018-05-21T05:04:32.7268460Z   File "/Users/vsts/agent/2.133.3/work/1/s/Lib/test/test_asyncio/test_base_events.py", line 1846, in run_loop
2018-05-21T05:04:32.7323360Z     return self.loop.run_until_complete(coro)
2018-05-21T05:04:32.7364920Z   File "/Users/vsts/agent/2.133.3/work/1/s/Lib/asyncio/base_events.py", line 566, in run_until_complete
2018-05-21T05:04:32.7382660Z     return future.result()
2018-05-21T05:04:32.7399140Z   File "/Users/vsts/agent/2.133.3/work/1/s/Lib/asyncio/selector_events.py", line 475, in sock_connect
2018-05-21T05:04:32.7423560Z     return await fut
2018-05-21T05:04:32.7440130Z   File "/Users/vsts/agent/2.133.3/work/1/s/Lib/asyncio/selector_events.py", line 480, in _sock_connect
2018-05-21T05:04:32.7457640Z     sock.connect(address)
2018-05-21T05:04:32.7477160Z OSError: [Errno 22] Invalid argument

@asvetlov
Copy link
Contributor

I hope the master is fixed now.
Don't know how to restart VSTS: don't see any "restart" button in UI.

vstinner
vstinner requested changes May 22, 2018
View reviewed changes
Copy link
Member
@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

I like the general idea, but I have complains about the current implementation.

Lib/test/libregrtest/save_env.py
asyncio.set_event_loop_policy(policy)

def get_asyncio_get_event_loop(self):
return asyncio.get_event_loop()
Copy link
Member

Choose a reason for hiding this comment

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

This function has a side effect: if there is no current event loop, it does create a new one. So I don't think that it's safe to call it. I don't think that we have an API to get the event loop without side effect.

Copy link
Member Author
@brettcannon brettcannon Jun 1, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Then basically the whole PR is pointless as every single thing here fetches the event loop somehow to check its state. (Which is fine, but we have to be aware that we the asyncio API is slightly deficient such that we can't do this).

Lib/test/libregrtest/save_env.py
)

def get_asyncio_get_event_loop_policy(self):
return asyncio.get_event_loop_policy()
Copy link
Member

Choose a reason for hiding this comment

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

This function creates a new policy if there is no current policy. The function has a side effect, so I don't think that it's ok to call it from here.

Lib/test/test_asyncgen.py
@@ -322,6 +322,7 @@ def test_async_gen_api_01(self):
class AsyncGenAsyncioTest(unittest.TestCase):

def setUp(self):
self.addCleanup(asyncio.set_event_loop, asyncio.get_event_loop())
8000
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because the tests were wiping out the event loop and were triggering the mutated environment check for the event loop.

Lib/test/test_contextlib_async.py
@@ -11,14 +11,15 @@ def _async_test(func):
"""Decorator to turn an async function into a test case."""
@functools.wraps(func)
def wrapper(*args, **kwargs):
old_loop = asyncio.get_event_loop()
Copy link
Member

Choose a reason for hiding this comment

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

Since asyncio.get_event_loop() API is weird, maybe an context manager helper should be added to test.support?

@bedevere-bot
Copy link

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

@vstinner
Copy link
Member

"I think that future.result is not thread-safe"

The whole class is not thread-safe:
https://docs.python.org/dev/library/asyncio-task.html#asyncio.Future
"This class is not thread safe."

@brettcannon
Copy link
Member Author

So it sounds like I should close this PR until there's a way to query if a global event loop has been created yet? Does that seem reasonable?

@brettcannon
Copy link
Member Author

Actually, I just realized that I'm complicating this. I can simply check if asyncio.events._event_loop_policy is None or not and ignore everything else since there simply shouldn't be a loop policy if a test properly cleaned up after itself since the event loop is stored on the event policy. So if I took Victor's idea and added a test.support.maybe_get_event_loop_policy() and have that just return _event_loop_policy, then it would be easy to detect when someone didn't clean up. And then resetting is is as simple as asyncio.events.set_event_loop_policy(None).

@vstinner
Copy link
Member
vstinner commented Jun 1, 2018

Python 3.7 added asyncio.get_running_loop() which doesn't create an event loop if there is no current loop. Maybe if you use this one, it will be fine?

@1st1
Copy link
Member
1st1 commented Jun 1, 2018

Python 3.7 added asyncio.get_running_loop() which doesn't create an event loop if there is no current loop. Maybe if you use this one, it will be fine?

get_running_loop() is supposed to be used only from within a coroutine, an asynchronous task, or a callback.

@brettcannon
Copy link
Member Author

I have another PR that I'm just waiting on the final test run to finish that should take care of everything to everyone's satisfaction.

@brettcannon
Copy link
Member Author

I've opened GH-7328 to supercede this PR.

@brettcannon brettcannon closed this Jun 1, 2018
@brettcannon brettcannon deleted the check-for-asyncio-mutations branch June 1, 2018 21:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vstinner vstinner vstinner requested changes

@1st1 1st1 Awaiting requested review from 1st1

Assignees
No one assigned
Labels
awaiting changes DO-NOT-MERGE tests Tests in the Lib/test dir topic-asyncio
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@brettcannon @1st1 @willingc @asvetlov @bedevere-bot @vstinner @the-knights-who-say-ni
0