gh-123290: fix reference leaks in curses

[picnixz]

This will superseed #123291. With this PR, the curses module does not leak anymore 🥳

• Issue: Multiple reference leaks in curses error-branches #123290

[bedevere-bot]

🤖 New build scheduled with the buildbot fleet by @picnixz for commit 112a62b 🤖

If you want to schedule another build, you need to add the 🔨 test-with-refleak-buildbots label again.

[vstinner]

@erlend-aasland@encukou@ericsnowcurrently: This PR converts the _curses extension to multiphase initialization API (PEP 489) and convert static types to heap types. Do you recall if we decided to leave the _curses extension unchanged on purpose? Or is it ok to have multiple instances of this extension?

[picnixz]

By the way, with this PR and #123285, no modules from the standard library (except those in tests, but that's because the _testcapi is deliberately using single-phase initialization) should leak references when imported (at least on POSIX).

[encukou]

Do you recall if we decided to leave the _curses extension unchanged on purpose?

I guess it's not very useful outside the main interpreter, so it was left behind.

I haven't looked at the code in detail, but, _curses manages process-global state (the terminal & the curses API around it), so it should:

• ideally: maintain process-global state, with all Python-level objects being “proxies” to that ([edit] i.e. global state is not stored in module-level state)
• or: only be loadable once per process

But neither had priority, so it's left with old behaviour that's somewhat broken with multiple interpreters.

The refleaks are one-time, right? Or can you leak an unlimited amount of memory by re-loading the module?

[picnixz]

The refleaks are one-time, right? Or can you leak an unlimited amount of memory by re-loading the module?

Mmh I don't think so actually (or at least I didn't manage to) [so yes, they are one-time]

[picnixz]

ideally: maintain process-global state, with all Python-level objects being “proxies” to that (i.e. module-level state)

I have a module's state but I'm not sure this is what you have in mind.

But neither had priority, so it's left with old behaviour that's somewhat broken with multiple interpreters.

Would {Py_mod_multiple_interpreters, Py_MOD_MULTIPLE_INTERPRETERS_NOT_SUPPORTED} be sufficient to indicate that it's not supported with multiple interpreters?

I guess it's not very useful outside the main interpreter, so it was left behind.

I'm not sure if this could matter, but for extensions that could be using stdlib modules, wouldn't it be better if they could detect whether they have refleaks by running python -X showrefcount? that way if there is a reference leak then the extension could assume that it's likely one their side and not on CPython's side (we could still have refleaks on our side but it's maybe better to reduce them as much as possible).

[encukou]

I have a module's state but I'm not sure this is what you have in mind.

Sorry for the truncated message! I meant global state is not stored in module-level state. (edited above)

Would {Py_mod_multiple_interpreters, Py_MOD_MULTIPLE_INTERPRETERS_NOT_SUPPORTED} be sufficient to indicate that it's not supported with multiple interpreters?

Yes.
I'd prefer the global flag, which means you can load it in any interpreter, not just the main one, but Py_MOD_MULTIPLE_INTERPRETERS_NOT_SUPPORTED is fine.

wouldn't it be better if [extensions] could detect whether they have refleaks by running python -X showrefcount?

Yes. I'm not talking about whether this is a valid issue, but about priorities :)

[vstinner]

This PR does "everything" at once, so it's hard to review. I would prefer to split the PR into sub-steps:

• Cleanup the code, prepare PyInit__curses()
• Add a module state
• Convert static types to heap types

[vstinner]

You should move these macros just before their first use, I don't think that it's useful to have them at the top.

[picnixz]

Actually, I put them in the "/* Utility Macros */" section where the other macros are being declared.

[vstinner]

I suggest adding an empty line (after return, before the label) for readability.

[picnixz]

This PR does "everything" at once, so it's hard to review. I would prefer to split the PR into sub-steps:

Sorry for that :( Do you want me to split it into multiple PRs or multiple commits?

[picnixz]

I meant global state is not stored in module-level state

What would the the global state consists of? Currently, what's being stored is the type of curses window (PyCursesWindow_Type) and the exception type (PyCursesError). The curses window type is also exported to the PyCurses_API and there is a global variable for PyCursesError because some functions need it but do not have access to the module's state.

Those functions are exported in the PyCurses_API and test whether curses has been initialized or not. I'm not sure whether I did something wrong because I'm wondering now whether it's possible for them to outlive the module's state, in which case calling the following macro could fail:

#definePyCursesSetupTermCalled \ if (initialised_setupterm != TRUE){\ PyErr_SetString(PyCursesError, \ "must call (at least) setupterm() first"); \ return 0}

So maybe we need to live with those refleaks?

[vstinner]

Do you want me to split it into multiple PRs or multiple commits?

Multiple PRs. Start by just creating one, others will follow once the first is merged.

[picnixz]

Closing this one since I'll split it in different PRs (and to reduce the number of opened PRs)