gh-123961: make curses global flags free-threaded friendly - #125000
gh-123961: make curses global flags free-threaded friendly#125000picnixz wants to merge 5 commits into
Conversation
Sorry, something went wrong.
…-threading-friendly-123961
ZeroIntensity
left a comment
There was a problem hiding this comment.
I couldn't find anything in here to complain about 😉, LGTM!
Sorry, something went wrong.
If you cannot have two instances of the _curses extension, what's the point of using atomic variables? |
Sorry, something went wrong.
From what I understand, it'd be possible to access the curses extension state from different threads (not necessarily processes). Or am I wrong? Ok I know why I didn't get the error in the terminal; it's because the module is still single-phase for now. Once it's multi-phase, I'll also get an error if I want to load it more than once per process (namely, I won't need to use the _testembed program). |
Sorry, something went wrong.
|
I suggest to first make the _curses extension use the multiphase init API, and then look on how to make it safe to be used on a Free Threaded build. |
Sorry, something went wrong.
|
Hmm, multiphase init shouldn't affect this. IIUC, module states need to be locked for free-threaded extensions. |
Sorry, something went wrong.
colesbury
left a comment
There was a problem hiding this comment.
I'm not familiar with the curses module, but I'm a bit skeptical of this approach. Is curses thread safe?
Sorry, something went wrong.
| static int | ||
| cursesmodule_exec(PyObject *module) | ||
| { | ||
| if (_Py_atomic_load_int(&curses_module_loaded)) { |
There was a problem hiding this comment.
The atomics here don't really do anything useful because you are not atomically checking and setting the variable together. Either use an atomic compare-exchange or don't use atomics at all.
Within an interpreter, the import lock will prevent multiple concurrent executions of cursesmodule_exec, but I'm not sure about what happens with multiple interpreters.
Sorry, something went wrong.
There was a problem hiding this comment.
FWIW, curses doesn't support subinterpreters, as indicated by the m_size being -1
Sorry, something went wrong.
There was a problem hiding this comment.
Within an interpreter, the import lock will prevent multiple concurrent executions of cursesmodule_exec
Oh ok. Thanks for the tip. I wasn't really sure on this one. I'll revert this change later
Sorry, something went wrong.
AFAIK, no (at least ncurses is NOT thread-safe). However, curses functions require some ncurses functions to be called at most once in order to work properly. We detect this by setting global flags. I'm clearly not an expert in free-threading nor in concurrency in general, so I really appreciate your help! |
Sorry, something went wrong.
I think it's worth figuring out what sort of locking and thread-safety strategy we should use for curses before using atomic operations on the global variables. For example, if we want module-level locks for most of the APIs then we probably won't want to use atomic operations if the variables are only accessed from within the locks. |
Sorry, something went wrong.
|
I think I'll close this PR for now. We'll likely need to revisit this question in a separate issue and PRs. |
Sorry, something went wrong.
We allow global variables indicating whether the underlying curses functions have been called to be safely used in free-threaded builds.
In addition, since curses manages the screen, only one curses module can be loaded per process (i.e the exec() function is executed only once and an ImportError is raised if the user attempts to load multiple curses modules within the same process).
This can be tested via:
./python -c "import sys; import subprocess; subprocess.run(['./Programs/_testembed', 'test_repeated_init_exec', 'import curses'])"