Closed tacaswell closed 1 year ago
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.
That looks like the tests are leaking event loops....
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.
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()
.
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.
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.....
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 / https://github.com/python/cpython/issues/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.
Note that set_event_loop()
will likely be deprecated as well: https://github.com/python/cpython/issues/94597
@jluebbe : That will probably be fixed after https://github.com/prompt-toolkit/python-prompt-toolkit/pull/1721 gets merged.
closes #1696