Skip to content

Conversation

@picnixz
Copy link
Member

@picnixzpicnixz 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
MemberAuthor

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.

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

This comment was marked as resolved.

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

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

@vstinnervstinner 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
MemberAuthor

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

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

importsysimportgcprint("load-unload") import_cursesdel_cursesdelsys.modules['_curses'] gc.collect() print("load again") import_cursesdel_cursesdelsys.modules['_curses']

@picnixz
Copy link
MemberAuthor

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

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
MemberAuthor

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 <<EOMimport sysprint("load-unload")import _cursesdel _cursesdel sys.modules['_curses']print("load again")import _cursesdel _cursesdel 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 <<EOMimport sysimport gcprint("load-unload")import _cursesdel _cursesdel sys.modules['_curses']gc.collect()print("load again")import _cursesdel _cursesdel sys.modules['_curses']EOM $ ./python -c "$code" load-unload load again

@vstinnervstinner merged commit e4292c0 into python:mainOct 8, 2024
@vstinner
Copy link
Member

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

@vstinner
Copy link
Member

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] 

@picnixzpicnixz 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 freeto 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

@picnixz@vstinner