-
-
Notifications
You must be signed in to change notification settings - Fork 31.8k
Refactorization of _cursesmodule.c
to fix reference leaks
#123961
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
Labels
Comments
Task 1 completed in #123962. |
_cursesmodule.c
_cursesmodule.c
vstinner
pushed a commit
that referenced
this issue
Sep 13, 2024
FTR, I started implementing the module's state. I'll push something tomorrow or in two days. |
This was referenced Sep 28, 2024
vstinner
pushed a commit
that referenced
this issue
Sep 29, 2024
vstinner
pushed a commit
that referenced
this issue
Oct 3, 2024
vstinner
pushed a commit
that referenced
this issue
Oct 4, 2024
This was referenced Oct 4, 2024
vstinner
pushed a commit
that referenced
this issue
Oct 8, 2024
efimov-mikhail
pushed a commit
to efimov-mikhail/cpython
that referenced
this issue
Oct 9, 2024
_cursesmodule.c
_cursesmodule.c
to fix reference leaks
This issue has many attached PRs but the main goal has been achieved (namely fixing reference leaks). I'll close this one as completed. For the free-threaded issue, we'll need a separate issue. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
Feature or enhancement
gh-123962: global dictionary removal
In
_cursesmodule.c
, there is a global reference to the module's dictionary. The reason is that we cannot add all constants when the module is being initialized because some constants requirecurses.initscr()
to be called beforehand.Instead of storing the module's dictionary in a global variable, we can access it in
curses.initscr()
usingPyModule_GetDict
.gh-124047: global variable name cleanup
Some global variables are modified in
_cursesmodule.c
by other functions and their names can be confused with a local variable name. I wanted to use uppercase names but Victor argued that this should be reserved for C macros (#123910 (comment)) and suggested keeping prefixed lowercase names.gh-124729: use a global state object to hold global variables
Once we are done with the above changes, we introduce a global module's state object. This state will hold the error and window type (which needs to be converted into a heap type) that are currently global variables.
gh-124907: improve C API cleanup
We improve the interface for creating and destroying the underlying C API and its associated capsule. Once we transform the Window type into a heap type, we'll also add the corresponding
traverse()
andclear()
callbacks to the capsule object.gh-124934: transform window type into a heap type
In this task, we make curses window type a heap type instead of a static one. Since the module is still not yet a multi-phase initialization module, we cannot entirely clean-up all references.
gh-125000: make the module
free-threaded friendlyprocess-wideThe module should not be allowed to be exec'ed() more than once by process. This is achieved by adding a global flag.
In addition, we should make the read/write operations on tbe global flags atomic so that free-threaded builds do not have race conditions.gh-124965: implement PEP-489 for
curses
The last task takes care of changing the module into a PEP-489 compliant one. It will also take care of the long-standing reference leaks in curses.
Some extra and optional tasks at the end:
Error messages improvements
Some functions say that "X() returned ERR" but the function X is not always the correct one (e.g., in
_curses_window_echochar_impl
, it says thatechochar
returned ERR but this depends on whether we usedechochar
orwechochar
). I think we need to really use the name of the function that was called at runtime (some functions do this but not all).The idea is to extend the exception type so that it also has two attributes, namely
called_funcname
andcurses_funcname
(subject to bikeshedding). Thecalled_funcname
value would contain the incriminating function (i.e. where the exception was raised) whilecurses_funcname
would contain the runtime C function that was used (which may differ in many cases from the actual curses Python function).To that end, we need the exception type to be a heap type, since a static type would be 1) an overkill 2) still leaks (and we don't like leaks).
Cosmetic changes
I can't say I'm maintaining curses but I can say that it's tiring to work with a file of more 5k lines. Many lines are actually caused by the fact that we have both the implementation for the window object and for the module's method. But these two can probably be split into two files. I don't know if this is suitable but it would probably help maintining the code (both take roughly 2k lines of code). Here are the various cosmetic points I'd like to address:
Split the module into multiple.c
files. This is not necessarily mandatory because we would probably need to export some functions that are shared or add some internal header so it might overcomplicate the task.func(y,x, something)
, it's hard to read).Those cosmetic changes could be helpful to newcomers, since no one is really interested in this module. But those are just things I had in mind while refactoring the module, none of them are really required (and I'm now quite used to).
EDIT (03/10/24)
I'll just give up on splitting the files. There are too many inter-dependencies between functions that it makes any PR for that too hard to review. In addition, we would end up exporting too many un-necessary symbols.
Related Work
Linked PRs
ModDict
in_cursesmodule.c
#123962curses
prefix to global variable names in_cursesmodule.c
#124047PyCursesError
in_cursesmodule.c
to a global state variable #124729curses.window
static type into a heap type #124934curses
to a multi-phase initialization module (PEP-489) #124965curses
global flags free-threaded friendly #125000The text was updated successfully, but these errors were encountered: