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

gh-123961: convert curses to a multi-phase initialization module (PEP-489) #124965

Merged
merged 10 commits into from
Oct 8, 2024

Conversation

picnixz
Copy link
Member

@picnixz picnixz commented Oct 4, 2024

I think this is the very last PR for this task, conditioned to some final cleanups just for macro readability and comments.

@picnixz
Copy link
Member Author

picnixz commented Oct 4, 2024

Following our discussion on the previous PR, I also changed the _cursesmodule prefix to cursesmodule so that we have at least a bit more of consistency. I won't touch the AC-generated functions.

@picnixz picnixz marked this pull request as draft October 4, 2024 10:35
@picnixz

This comment was marked as resolved.

@picnixz picnixz marked this pull request as ready for review October 4, 2024 11:28
@picnixz
Copy link
Member Author

picnixz commented Oct 4, 2024

I'm really un-sure of how free-threaded builds with multi-phase initialization would interact with capsule objects. So I'd happy if any free-threaded expert could help me here. Here are some questions (and we may perhaps address them in separate PRs):

  • Should the global flags (simple int) be atomically set?
  • Should we use PyRawMem_* instead of PyMem_* functions when allocating the memory for capsule objects?
  • Why am I only seeing the assertion failure on my pointer not NULL when I converted it into a multi-phase initialization module and not before? (see 9e0dd70 for the commit who actually "fixed" it).

cc @colesbury (I don't know other free-threaded experts but feel free to delegate the question to anyone who knows about the topic if you don't have enough time!)

Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you try to add a global variable to prevent having more than 1 instance at the same time?

@picnixz
Copy link
Member Author

picnixz commented Oct 7, 2024

Here's the output, do you think it's good enough?

>>> import sys
...
>>> import _curses as curses1
>>> del sys.modules['_curses']
...
>>> import _curses as curses2
...
Traceback (most recent call last):
  File "<python-input-3>", line 1, in <module>
    import _curses as curses2
ImportError: module 'curses' can only be loaded once per process

@vstinner
Copy link
Member

vstinner commented Oct 7, 2024

cursesmodule_free() should set curses_module_loaded to 0. So you can load-unload the extension multiple times:

import sys
import gc

print("load-unload")
import _curses
del _curses
del sys.modules['_curses']
gc.collect()

print("load again")
import _curses
del _curses
del sys.modules['_curses']

@picnixz
Copy link
Member Author

picnixz commented Oct 8, 2024

cursesmodule_free() should set curses_module_loaded to 0. So you can load-unload the extension multiple times:

I'm not sure it works. Even if I do this it still tells me that the curses module cannot be loaded more than once per process. The cursesmodule_free() does not seem to be called even if gc.collect() is called (at least not in the REPL). What should I do?

@vstinner
Copy link
Member

vstinner commented Oct 8, 2024

The cursesmodule_free() does not seem to be called even if gc.collect() is called (at least not in the REPL).

It works if you run my script:

load-unload
load again

@picnixz
Copy link
Member Author

picnixz commented Oct 8, 2024

After some tests, it appears that the REPL only calls free() upon exiting the process (the REPL imports curses in Lib/_pyrepl/curses.py). So, reloading curses inside the REPL won't work. In a normal script, however:

$ read -r -d '' code << EOM
import sys

print("load-unload")
import _curses
del _curses
del sys.modules['_curses']

print("load again")
import _curses
del _curses
del sys.modules['_curses']
EOM
$ ./python -c "$code"
load-unload
load again
Traceback (most recent call last):
  File "<string>", line 9, in <module>
    import _curses
ImportError: module 'curses' can only be loaded once per process

When free is called via gc.collect():

$ read -r -d '' code << EOM
import sys
import gc

print("load-unload")
import _curses
del _curses
del sys.modules['_curses']
gc.collect()

print("load again")
import _curses
del _curses
del sys.modules['_curses']
EOM
$ ./python -c "$code"
load-unload
load again

@vstinner vstinner merged commit e4292c0 into python:main Oct 8, 2024
37 checks passed
@vstinner
Copy link
Member

vstinner commented Oct 8, 2024

Merged, thanks. That's a nice step forward for the _curses extension :-)

@vstinner
Copy link
Member

vstinner commented Oct 8, 2024

Python no longer leaks at exit:

$ ./python -X showrefcount -c 'import _curses'
[0 refs, 0 blocks]

Python 3.13 for comparison:

$ ./python -X showrefcount -c 'import _curses'
[234 refs, 159 blocks]

@picnixz picnixz deleted the curses/pep-489-123961 branch October 8, 2024 11:50
efimov-mikhail pushed a commit to efimov-mikhail/cpython that referenced this pull request Oct 9, 2024
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.

2 participants