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

fix: remove the use of asyncio.get_child_watcher() for python3.14 #584

Merged
merged 1 commit into from
Jan 13, 2025

Conversation

cryptomilk
Copy link
Contributor

Use PidfdChildWatcher if os.pidfd_open is available and works, and otherwise use ThreadedChildWatcher which should work in any case.

Fixes #583

@cryptomilk
Copy link
Contributor Author

Looks like that the CI fails before it runs the tests.

@justinmk
Copy link
Member

justinmk commented Jan 3, 2025

expand this to see the test failures:

image
 FAILED test/test_buffer.py::test_options - OSError: EOF
  FAILED test/test_buffer.py::test_get_exceptions - OSError: EOF
  FAILED test/test_window.py::test_options - OSError: EOF

@cryptomilk cryptomilk force-pushed the asn-fix branch 2 times, most recently from 3b95c0a to accf17f Compare January 4, 2025 08:35
@wookayin
Copy link
Member

wookayin commented Jan 5, 2025

I will try to diagnose the test failure on python 3.13 with nvim-0.11 nightly and send a separate PR to fix it.

UPDATE: this is an upstream issue, see neovim/neovim#31894

@cryptomilk
Copy link
Contributor Author

Oh, locally I'm running nvim 0.10.3 and everything works. GH runs nvim nightly?

@cryptomilk
Copy link
Contributor Author

I've rebased on master and it shows that the patch works fine with nvim-stable but fails with nvim-nightly.

@justinmk
Copy link
Member

justinmk commented Jan 7, 2025

windows ci shows an issue:

  C:\hostedtoolcache\windows\Python\3.13.1\x64\Lib\asyncio\unix_events.py:40: in <module>
      raise ImportError('Signals are not really supported on Windows')
  E   ImportError: Signals are not really supported on Windows

other than that, the failing vim.current.buffer.options['invalid-option'] tests are a known issue

Note: Tests are still failing on nvim-nightly due to upstream bug, but as soon as neovim/neovim#31894 is fixed CI should be back to green.

@cryptomilk
Copy link
Contributor Author

Maybe we need to make sure the ThreadedChildWatcher is used on Windoze?

@cryptomilk cryptomilk force-pushed the asn-fix branch 5 times, most recently from de3a116 to b91063f Compare January 7, 2025 15:21
@wookayin
Copy link
Member

wookayin commented Jan 8, 2025

CI on python3.7 reports:

RuntimeError: Cannot add child handler, the child watcher does not have a loop attached

whereas on all other python versions (>=3.8) CI failure is expected just because of Nvim crashing.

It'd be also appreciated if you could fix the commit message title with an appropriate header (e.g. fix: replace deprecated ...).

@cryptomilk
Copy link
Contributor Author

I'm sick, it has to wait ...

@justinmk
Copy link
Member

justinmk commented Jan 8, 2025

If necessary, we can drop python 3.7 support.

@cryptomilk
Copy link
Contributor Author

The error on 3.7 is now the same as with 3.13

@justinmk
Copy link
Member

justinmk commented Jan 9, 2025

neovim/neovim#31894 was fixed, will re-run the tests here after a new nightly is posted.

@cryptomilk
Copy link
Contributor Author

Rebased, tests are mostly passing now. The Windows and MacOS tests with Python 3.13 are failing.

On Windows it looks like we have some closing workaround which fails with Python 3.13 now.

# Windows: for ProactorBasePipeTransport, close() doesn't take in

I have no idea what is wrong on MacOS.

@cryptomilk
Copy link
Contributor Author

I added a commit which disabled the workaround on Windows and that seems to work. Not sure if this is the right fix. I have no clue about Windows.

The failing test_attach.py::test_connect_stdioThe test on MacOS seems to be race condition. Now it works on 3.13 and fails on 3.12.

@wookayin
Copy link
Member

I don't think the macOS failure has something to do with this PR; it'll be a different bug that already existed. I'll see later if we can bring a proper fix.

@justinmk
Copy link
Member

disabled the workaround on Windows and that seems to work

that was added in 17fbcbc ; @wookayin do you see any risk with disabling it here?

@wookayin
Copy link
Member

I cherry-picked the commit eb14e83 for bumping up msgpack as it's a bit tangential to this fix.

Regarding the workaround in 17fbcbc, we can try disabling the workaround for python3.13 on Windows to see if the bug has been fixed. But I believe for older versions this workaround will still be necessary.

@cryptomilk cryptomilk force-pushed the asn-fix branch 2 times, most recently from 3dc3074 to 856bf92 Compare January 10, 2025 20:15
@cryptomilk cryptomilk requested a review from wookayin January 10, 2025 20:18
@cryptomilk
Copy link
Contributor Author

I really start to hate Python.

python/cpython@9d2e1ea
python/cpython#120804

Every 2 versions they completely break the API. WTF.

@cryptomilk
Copy link
Contributor Author

Looks like supporting 3.14 is a much bigger rewrite.

@wookayin
Copy link
Member

wookayin commented Jan 10, 2025

I appreciate your patience. Now as I think about the role of child_watcher, it's not quite crucial part of the pynvim functionality and I think we actually don't need it for other than preventing resource leak and some unwanted side effects (fd4247c) when the event loop closes.

Let's make our life simpler and easier: my proposal is to simply just disable child_watcher in python3.14 (i.e. set child_watcher = None), and let pynvim just work for python3.14. (provided that test passes...)

@cryptomilk
Copy link
Contributor Author

cryptomilk commented Jan 11, 2025

Looks like I misunderstood how things are now. I hope the latest changes are correct now.

@wookayin wookayin changed the title Replace deprecated asyncio.get_child_watcher() fix: remove the use of asyncio.get_child_watcher() for python3.14 Jan 11, 2025
@justinmk justinmk merged commit e2a3ead into neovim:master Jan 13, 2025
24 of 25 checks passed
@justinmk
Copy link
Member

Thank you both!

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.

broken on python3.14 ('asyncio' has no attribute 'get_child_watcher')
3 participants