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

Release 0.14 breaks tests that rely on a specific event_loop_policy #168

Closed
wouterdb opened this issue Jun 25, 2020 · 13 comments · Fixed by #192
Closed

Release 0.14 breaks tests that rely on a specific event_loop_policy #168

wouterdb opened this issue Jun 25, 2020 · 13 comments · Fixed by #192

Comments

@wouterdb
Copy link

wouterdb commented Jun 25, 2020

Problem

We have many testcases that rely on asyncio.set_event_loop_policy(AnyThreadEventLoopPolicy())
We set the event loop policy in the conftest.py, such that it is set before any tests start.

Since 0.14 these testcases produce errors

RuntimeError: There is no current event loop in thread 'ThreadPoolExecutor-109_6'.

Root cause

The change here: https://github.com/pytest-dev/pytest-asyncio/pull/164/files#diff-4d85e8313d8e67cb777137411a11ac86R80

Resets the event loop policy after the first test, breaking all the next tests.

@Tinche
Copy link
Member

Tinche commented Jun 27, 2020

@wouterdb will dig into this over the weekend!

@wouterdb
Copy link
Author

wouterdb commented Jun 27, 2020 via email

@alblasco
Copy link
Contributor

Thanks also for your kind feedback. I did V0.14 changes you are referring, and I was not fully aware of your user case.

With this in mind and after some debugging, I see no problem to roll back some of V0.14 changes:
a) Restore old_loop = policy.get_event_loop() and its finalizer fixturedef.addfinalizer(lambda: policy.set_event_loop(old_loop))
https://github.com/pytest-dev/pytest-asyncio/pull/164/files#diff-4d85e8313d8e67cb777137411a11ac86L58-L65
b) Remove finalizer: asyncio.set_event_loop_policy(None) https://github.com/pytest-dev/pytest-asyncio/pull/164/files#diff-4d85e8313d8e67cb777137411a11ac86R75-R80

@Tinche With these changes all our unitary tests keep passing.
BTW. It seems that skipping V0.13, has not been enough to avoid issues.

@Tinche
Copy link
Member

Tinche commented Jun 28, 2020

Yeah, it would be great if we could treat the loop policy in the plugin as read-only (i.e. use it for generating event loops but let users override it). Another use case I imagine is very common is testing with UVLoop.

@Tinche
Copy link
Member

Tinche commented Jun 28, 2020

@alblasco I've done some work on the 'policies' branch, but I still have trouble making all the tests pass. The first test in a package still uses the wrong event loop for some reason.

alblasco added a commit to alblasco/pytest-asyncio that referenced this issue Jun 28, 2020
To take into account user case that sets a specific event loop policy before runnins pytest asyncio tests:
- Rollover changes in V0.14 (that clears event loop policy in pytest_fixture_post_finalizer)
- include a new unit test that checks if policy has been changed (fails with V0.14 and pass with this PR).
@Tinche
Copy link
Member

Tinche commented Jun 28, 2020

@wouterdb I'm doing some work to support this use case over at #174.

Due to a lot of users using the plugin in a multitude of ways, it's not super trivial to fix. The fix I'm proposing for now is to treat fixtures named event_loop_policy a special way (like we treat fixtures called event_loop a special way). Then we can ensure those fixtures are executed at the right times. Would this work for you? I guess you would have to set the policy using a fixture and that fixture needs to be named event_loop_policy.

@wouterdb
Copy link
Author

This would work for us. (It would actually be a lot cleaner than what we do now.)

Thank you for you fast response.

@bryevdv
Copy link

bryevdv commented Oct 4, 2020

I guess you would have to set the policy using a fixture and that fixture needs to be named event_loop_policy.

I'm afraid I don't follow all the discussion, but this sentence seems troublesome. In our case the library itself is setting a policy (because we utilize tornado which cannot use the default proactor on windows). I maintain that we should not have to do or set anything at all in our tests in order to have the library-configured policy be respected as-is (otherwise, we fail to be able to test that the library itself is configuring the policy, as it must do).

@wouterdb
Copy link
Author

wouterdb commented Oct 5, 2020

we should not have to do or set anything at all in our tests in order to have the library-configured policy be respected as-is

To us, it would be good if pytest doesn't touch the policy (as it did before), but if it does reset it, I would prefer to be able to control what it is reset to.

The problem for us is that pytest now resets the event_loop_policy after every test.
Many of our unit tests require an AnyThreadEventLoopPolicy. Many of our fixtures are asynchronous as well.
So by the time the actual test starts, the event loop is already in use. Changing the event_loop_policy when the event loop is already in use is very messy and causes complicated errors:

(otherwise, we fail to be able to test that the library itself is configuring the policy, as it must do).

The reverse side of this is that we have many unit test that don't configure the policy themselves (the code that configures the policy is out-of-scope for that test) so we need to be able to define a fixture that sets the policy before any of these test are executed.

Current work around

def patch_policy():
    # work around for https://github.com/pytest-dev/pytest-asyncio/issues/168
    oldloop = asyncio.get_event_loop_policy()
    if not isinstance(oldloop, AnyThreadEventLoopPolicy):
        newloop = AnyThreadEventLoopPolicy()
        # transfer existing eventloop to the new policy
        newloop.set_event_loop(oldloop.get_event_loop())
        asyncio.set_event_loop_policy(newloop)

@wouterdb
Copy link
Author

Is this issue still active?
We are still suffering from this problem.

Another symptom of this issue is subprocess no longer working, but perhaps that is because this bug triggers/interacts with #98 or sideways with #114.

 File "/usr/lib/python3.6/asyncio/subprocess.py", line 225, in create_subprocess_exec
    stderr=stderr, **kwds)
  File "/usr/lib/python3.6/asyncio/base_events.py", line 1222, in subprocess_exec
    bufsize, **kwargs)
  File "/usr/lib/python3.6/asyncio/unix_events.py", line 203, in _make_subprocess_transport
    self._child_watcher_callback, transp)
  File "/usr/lib/python3.6/asyncio/unix_events.py", line 867, in add_child_handler
    "Cannot add child handler, "
RuntimeError: Cannot add child handler, the child watcher does not have a loop attached
During handling of the above exception, another exception occurred:

@edvgui
Copy link

edvgui commented Apr 29, 2021

The work around mentioned above by @wouterdb worked for me too!

But it would definitely be nice to see a real solution to this issue.

@bryevdv
Copy link

bryevdv commented Apr 29, 2021

FWIW we still pin to pytest-asyncio 0.12.0 but only for Python 3.8+ it seems. The pin was removed for earlier Python versions and awhile back and things seem to be working. I'm not sure if things would work now if the pin was removed on the later python versions, will have to try when time permits.

@Tinche
Copy link
Member

Tinche commented Apr 29, 2021

I worked on this a little but abandoned it, can't remember why now since it's been a while.

If you override the event_loop fixture (not the event_loop_policy fixture), can you just set the event loop policy in there?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants