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

MNT: Avoid warning on 3.10.9+, 3.11.1+, and 3.12.0+ #1714

Merged

Conversation

tacaswell
Copy link
Contributor

closes #1696

@carlwgeorge
Copy link

I'm not a maintainer here but I do maintain the Fedora package of this library. I ran the tests with this PR on Python 3.12.0a4, and there is another exception.

(.py312) [carl@teal:~/development/prompt-toolkit:mnt_loop_deprecation]$ pytest -x
======================================= test session starts =======================================
platform linux -- Python 3.12.0a4, pytest-7.2.1, pluggy-1.0.0
rootdir: /home/carl/development/prompt-toolkit
collected 145 items                                                                               

tests/test_async_generator.py .                                                             [  0%]
tests/test_buffer.py .........                                                              [  6%]
tests/test_cli.py ............F

============================================ FAILURES =============================================
______________________________ test_emacs_arguments_for_all_commands ______________________________

    def get_event_loop() -> asyncio.AbstractEventLoop:
        """Backward compatible way to get the event loop"""
        # Python 3.6 doesn't have get_running_loop
        # Python 3.10 deprecated get_event_loop
        if sys.version_info >= (3, 7):
            getloop = asyncio.get_running_loop
        else:
            getloop = asyncio.get_event_loop
    
        try:
>           return getloop()
E           RuntimeError: no running event loop

src/prompt_toolkit/eventloop/utils.py:116: RuntimeError

During handling of the above exception, another exception occurred:

    def test_emacs_arguments_for_all_commands():
        """
        Test all Emacs commands with Meta-[0-9] arguments (both positive and
        negative). No one should crash.
        """
        for key in ANSI_SEQUENCES:
            # Ignore BracketedPaste. This would hang forever, because it waits for
            # the end sequence.
            if key != "\x1b[200~":
                try:
                    # Note: we add an 'X' after the key, because Ctrl-Q (quoted-insert)
                    # expects something to follow. We add an additional \r, because
                    # Ctrl-R and Ctrl-S (reverse-search) expect that.
>                   result, cli = _feed_cli_with_input("hello\x1b4" + key + "X\r\r")

tests/test_cli.py:402: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
tests/test_cli.py:60: in _feed_cli_with_input
    _ = session.prompt()
src/prompt_toolkit/shortcuts/prompt.py:1034: in prompt
    return self.app.run(
src/prompt_toolkit/application/application.py:969: in run
    loop = get_event_loop()
src/prompt_toolkit/eventloop/utils.py:118: in get_event_loop
    loop = asyncio.new_event_loop()
/usr/lib64/python3.12/asyncio/events.py:797: in new_event_loop
    return get_event_loop_policy().new_event_loop()
/usr/lib64/python3.12/asyncio/events.py:694: in new_event_loop
    return self._loop_factory()
/usr/lib64/python3.12/asyncio/unix_events.py:64: in __init__
    super().__init__(selector)
/usr/lib64/python3.12/asyncio/selector_events.py:63: in __init__
    selector = selectors.DefaultSelector()
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

self = <selectors.EpollSelector object at 0x7f6707539100>

    def __init__(self):
        super().__init__()
>       self._selector = self._selector_cls()
E       OSError: [Errno 24] Too many open files

/usr/lib64/python3.12/selectors.py:349: OSError
===================================== short test summary info =====================================
FAILED tests/test_cli.py::test_emacs_arguments_for_all_commands - OSError: [Errno 24] Too many open files
!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! stopping after 1 failures !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
================================== 1 failed, 22 passed in 7.59s ===================================

To be clear the tests already don't pass on Python 3.12 on the master branch.

@tacaswell
Copy link
Contributor Author

That looks like the tests are leaking event loops....

@jonathanslenders
Copy link
Member

The idea is to transition soon to using asyncio.run() in prompt_toolkit.application.Application.run() and never call set_event_loop or new_event_loop.

In the meantime, I can merge this and we can look at the leaking event loops in the tests.

@jonathanslenders jonathanslenders merged commit 03db68a into prompt-toolkit:master Jan 31, 2023
@jonathanslenders
Copy link
Member

What do you think about dropping Python 3.6 support? It would simplify dealing with these issues. 3.6 is end of life for over a year now. That way, we can have get_running_loop() everywhere and use asyncio.run().

@tacaswell tacaswell deleted the mnt_loop_deprecation branch January 31, 2023 23:04
@tacaswell
Copy link
Contributor Author

As one of the authors of https://numpy.org/neps/nep-0029-deprecation_policy.html I am very 👍🏻 on dropping older Pythons (which as of last month says the support window is 3.8+)!

I can see that maybe being a bit aggressive for prompttoolkit, but I think dropping support for versions of Python that are no longer supported by upstream should be completely non-controversial.

@tacaswell
Copy link
Contributor Author

To be clear the tests already don't pass on Python 3.12 on the master branch.

I can not reproduce this. Bisecting the failure with py3.10.9 points to this commit as being the problem.

Trying with py3.9.16 after this is merged I am also seeing things like

Exception ignored in: <function BaseEventLoop.__del__ at 0x7f66a15a65e0>
Traceback (most recent call last):
  File "/usr/lib/python3.9/asyncio/base_events.py", line 688, in __del__
    self.close()
  File "/usr/lib/python3.9/asyncio/unix_events.py", line 58, in close
    super().close()
  File "/usr/lib/python3.9/asyncio/selector_events.py", line 87, in close
    self._close_self_pipe()
  File "/usr/lib/python3.9/asyncio/selector_events.py", line 94, in _close_self_pipe
    self._remove_reader(self._ssock.fileno())
AttributeError: '_UnixSelectorEventLoop' object has no attribute '_ssock'

from pytest clean up which makes me think there is actually something quite wrong with this patch.....

@tacaswell
Copy link
Contributor Author

On a bit more consideration, I suspect this should be reverted in favor of eating the exception or a more complex fix.

The problem is that get_running_eventloop() does what it says on the tin and returns the running event loop not the registered event loop so with this change we are effectively making a new event loop every time it is called out side of a running event loop.

Adding an lru_cache to this function helps, but I suspect that opens up a whole bunch of threading nonsense (which is why the standard library using threading.local). However, even accounting for that, this would be a second cache which would not be out of sync with asyncio's state and while it would avoid the first warning, would mean it would miss later set_event_loop() calls.


The test that does the test suite in is test_emacs_arguments_for_all_commands() which creates ~480 event loops.


I "tested" this by running the application where I was seeing the issue not by running the test suite 😞 . I suspect it passes on CI because the machines are slow enough that GC saves us or there is a high enough file limit set someplace.


https://bugs.python.org/issue39529 / python/cpython#83710 are the upstream discussions of this. It seems the goal is to deprecate exactly this usage (to get a handle on the event loop before it is running, schedule some stuff, and then start it) so I am not sure there actually is a "good" path to this short of switching to asyncio.run everywhere.

@jluebbe
Copy link

jluebbe commented Feb 7, 2023

Note that set_event_loop() will likely be deprecated as well: python/cpython#94597

@jonathanslenders
Copy link
Member

@jluebbe : That will probably be fixed after #1721 gets merged.

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 this pull request may close these issues.

DeprecationWarning: There is no current event loop
4 participants