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

Push PyPy3 compatibility to 100% #58

Open
Technologicat opened this issue Jan 7, 2020 · 6 comments
Open

Push PyPy3 compatibility to 100% #58

Technologicat opened this issue Jan 7, 2020 · 6 comments
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@Technologicat
Copy link
Owner

Technologicat commented Jan 7, 2020

PyPy3 looks promising, as it's rumored to be fast, and it supports version 3.6.9 of "the Python standard". Our main target platform just so happens to be Python 3.6.x.

Our PyPy3 compatibility is already near 100%. This issue documents what remains to be done.

  • Test compatibility with PyPy. Most things should work already.
    • Yes, pretty much everything already works. All tests pass except async_raise.
  • Macros? Is the ast close enough to CPython's?
    • Yes, macros work.
  • unpythonic.misc.async_raise? This may or may not work, depending on what is available in PyPy3's ctypes module. (It's only needed for remote Ctrl+C in the REPL server, but that's a nice feature to have.)
    • Doesn't work in PyPy, and without hacking PyPy itself, currently no way to make it work. See details below.
  • Get rid of __del__ methods, since they may be delayed or not called at all, and in most cases having a __del__ is just not pythonic anyway.
    • The one in unpythonic.tco._jump doesn't (really) matter, since its only purpose is to print a warning.
    • The one in unpythonic.dynassign._DynLiveView is more serious. How to get around it? A with block is no solution here, since the _DynLiveView instance is returned by unpythonic.dyn.asdict, so it has indefinite dynamic extent, depending on the code that calls asdict. Requiring users to call a .close() method explicitly just invites omission by accident or even on purpose (e.g. who ever calls socket.shutdown()?). OTOH, maybe _DynLiveView could be a singleton? The dynamic scopes are already context-managed, and that's really all that matters. _DynLiveView singleton FTW. 1a0f0f5. How about for the lose, instead. Can't work, since the dynamic scope stacks are thread-local. Having observers linger around a bit longer than they absolutely need to, or not having them cleaned up when the process exits, are not really a problem. In practice, the old solution should be fine in PyPy, too. Hence, reverted in 089f7cc.
  • Anything else? Add more details here. Maybe refer to Python compatibility and Differences between PyPy and CPython.
@Technologicat Technologicat added the enhancement New feature or request label Jan 7, 2020
@Technologicat Technologicat added this to the milestone Jan 7, 2020
@Technologicat Technologicat self-assigned this Jan 7, 2020
@Technologicat
Copy link
Owner Author

Technologicat commented Jan 7, 2020

Ok, so PyPy doesn't havectypes.pythonapi, but it has cpyext, which at least sounds promising. Will have to look at this a bit more.


Hm, cpyext's pystate.py doesn't seem to have PyThreadState_SetAsyncExc. And stubs.py indeed confirms it's missing.

In CPython, the signal stdlib module docs suggest KeyboardInterrupt usually works somewhat differently: the process receives an OS-level SIGINT, which is caught by the default handler - in the main Python thread, since that's the only one that can handle OS-level signals in CPython - which then raises KeyboardInterrupt. Look ma, no async raise. The magic already happened when the main thread's execution ended up in the SIGINT handler, which then just needs to perform a regular raise.

That solution is not applicable to a REPL server - we really need an async raise to trigger the exception in the desired thread (the one that's running the REPL session we want to send the Ctrl+C to). So maybe a small addition to pystate.py in PyPy is what we need - if PyPy's internals allow us to implement the missing function.


Hm. Another approach: signal.pthread_kill with __pypy__.thread.signals_enabled, and a PyPy-only with block around the call to the REPL's interact?

It's not as general as the current CPython solution, since it can only do OS signals, not arbitrary exceptions, but that's good enough for what we want to do here. To make it general (just for feature parity), we could provide a mechanism that usurps e.g. SIGUSR1 to asynchronously send any exception via a global dictionary acting as a mailbox, but the user code still needs to enable OS signals on PyPy (specifically for the targeted block of code in the target thread) for that to work. OTOH, this is Python, where explicit is better than implicit. OTTH, that takes up one OS signal. OTFH, we can provide a decorator that adds the PyPy-specific with block (no-opping in CPython), so it's as easy as decorating the appropriate function - indeed, the thread entrypoint - with it.

The only unclear thing is what the PyPy docs mean by that a signal might be handled in either the main thread or one currently inside such a with block. Is PyPy's pthread_kill specific enough to hit the target thread? (On CPython, it's documented as not useful to try to target any Python thread but the main one.)


If we go the mailbox route, we should probably keep track of thread idents that have been marked as accepting async raises. Global dict, clean up in a finally clause inside the decorator; both the OS and Python are allowed to recycle thread idents.

This approach gives us a consistent async_raise API across CPython and PyPy. Requiring to mark threads that allow async_raise keeps things explicit - no need to worry about KeyboardInterrupt in threads other than the main one (as usual!), unless marked.

This way we can also know at the time async_raise is called whether the target thread advertises itself as accepting an async_raise, so we can fail-fast if given an inappropriate Thread instance.

@Technologicat
Copy link
Owner Author

Macros should be fine, MacroPy has been tested on PyPy.

@Technologicat
Copy link
Owner Author

Technologicat commented Jan 9, 2020

Quickly tested on PyPy 7.3.0 (runtests.py), all tests except async_raise pass.

REPL server/client seem to have a small problem with missing \r, when printing just \n. Need to investigate. EDIT: Implementing prompt detection in 2658ede fixed this, too.

But mostly, it's already working, including all macros!

@Technologicat
Copy link
Owner Author

Technologicat commented Jan 9, 2020

Meh. Tested, and it turns out pthread_kill isn't specific enough to always hit the targeted thread. The might in the docs really means that; it's basically random which signal-enabled thread gets to run the signal handler (in the sense of OS signals, not in the unrelated CL signal sense).

So it may be we really need to implement PyThreadState_SetAsyncExc in PyPy to get async_raise to work in PyPy3... not really looking forward to that experience.

@Technologicat
Copy link
Owner Author

Technologicat commented Jan 12, 2020

Regarding PyThreadState_SetAsyncExc:

  • Note the branch is py3.6, not the default branch. (The default branch in the PyPy repo is for continued support of 2.7, due to the RPython toolchain itself being based on a restricted dialect of 2.7.)
  • PyPy's executioncontext has a promising-sounding set_exception.
  • We already have the right function signature in stubs.py.
  • So the question is, how to grab the executioncontext of the correct thread. We get the thread ident as input, and the interpreter must store the thread contexts somewhere...

Meh, cpyext functions aren't even exposed to the Python level, unlike CPython's ctypes.pythonapi. Maybe PyPy3 doesn't currently have a natural place for this. May need to ask the devs about it.

@Technologicat
Copy link
Owner Author

Technologicat commented Feb 6, 2020

Alternative way to signal a particular thread: signalfd.

See some context.

Just need to test if it works in PyPy...

@Technologicat Technologicat changed the title Make unpythonic compatible with PyPy3 Push PyPy3 compatibility to 100% Feb 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

1 participant