Skip to content

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

Closed
picnixz opened this issue Sep 11, 2024 · 3 comments
Closed

Refactorization of _cursesmodule.c to fix reference leaks #123961

picnixz opened this issue Sep 11, 2024 · 3 comments
Assignees
Labels
extension-modules C modules in the Modules dir type-feature A feature request or enhancement

Comments

@picnixz
Copy link
Member

picnixz commented Sep 11, 2024

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 require curses.initscr() to be called beforehand.

Instead of storing the module's dictionary in a global variable, we can access it in curses.initscr() using PyModule_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() and clear() 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 friendly process-wide

The 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 that echochar returned ERR but this depends on whether we used echochar or wechochar). 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 and curses_funcname (subject to bikeshedding). The called_funcname value would contain the incriminating function (i.e. where the exception was raised) while curses_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:

  • Make the macros more readable (there are packed and honestly, for a first-time contributor, hard to parse).
  • 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.
  • Simplify some code paths (just some if/else that could be cleaner or things like that).
  • Put whitespaces in function calls (honestly, when you see 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

@picnixz picnixz added the type-feature A feature request or enhancement label Sep 11, 2024
@picnixz picnixz self-assigned this Sep 11, 2024
@picnixz picnixz added the extension-modules C modules in the Modules dir label Sep 11, 2024
@picnixz picnixz moved this to Bugs in Curses issues Sep 11, 2024
@picnixz picnixz moved this from Bugs to Features in Curses issues Sep 11, 2024
@picnixz
Copy link
Member Author

picnixz commented Sep 13, 2024

Task 1 completed in #123962.

@picnixz picnixz closed this as completed Sep 13, 2024
@github-project-automation github-project-automation bot moved this from Features to Done in Curses issues Sep 13, 2024
@picnixz picnixz changed the title Avoid global module's dictionary variable in _cursesmodule.c Refactorization of _cursesmodule.c Sep 13, 2024
@picnixz picnixz reopened this Sep 13, 2024
@picnixz picnixz moved this from Done to Features in Curses issues Sep 13, 2024
vstinner pushed a commit that referenced this issue Sep 13, 2024
…c` (#124047)

Use the `const char*` type instead of a `const *` for the encoding name.
@picnixz
Copy link
Member Author

picnixz commented Sep 27, 2024

FTR, I started implementing the module's state. I'll push something tomorrow or in two days.

@picnixz picnixz changed the title Refactorization of _cursesmodule.c Refactorization of _cursesmodule.c to fix reference leaks Oct 14, 2024
@picnixz
Copy link
Member Author

picnixz commented Oct 14, 2024

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.

@picnixz picnixz closed this as completed Oct 14, 2024
@github-project-automation github-project-automation bot moved this from Features to Done in Curses issues Oct 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
extension-modules C modules in the Modules dir type-feature A feature request or enhancement
Projects
Status: Done
Development

No branches or pull requests

1 participant