It seems the leak is with SEM_VALUE_MAX, a static integer?
That could be solved by a PyGetSetDef getter.

Repeating #94336 (comment): AFAICS, the multiprocessing types do not access global module state, so there should be no reason to convert them to heap types.

It seems the leak is with SEM_VALUE_MAX, a static integer?

Where is that declared static? I don't see how it is considered a static integer. I am happy to be wrong though.

I changed the issue title. It seems to me that the issue is a ref leak. The proposed solution is converting types to heap types. IMO, the GitHub Issue title should reflect the problem, not necessary the solution.

It seems to me that the issue is a ref leak.

Do you have a fix that does not use heap types or private APIs but fixes the issue?

Ah, terminology. It's not C static, but it's always the same, and it's currently meant to be initialized only once (and it's not decref'd, so it leaks). You can't change the value from Python.

Do you have a fix that does not use heap types or private APIs but fixes the issue?

I can send one later today, if nothing comes up.

I can send one later today, if nothing comes up.

Sure, please add me as a reviewer, I'll learn something new.

OK, I see now that the leak is not caused by SEM_VALUE_MAX, but (mostly?) by the method/getter/setter wrappers set by PyType_Ready.
It seems to me that these are harmless: they won't be created a second time when _multiprocessing is imported again, and they can be reused in other interpreters. OTOH, they are flagged by -X showrefcount, and it would be nice to get those numbers to zero so -X showrefcount is useful in flagging new issues.

@vstinner, are you aware of this issue with -X showrefcount and static types with methods/getters/setters?

In any case, the module state is unnecessary here.

In any case, the module state is unnecessary here.

Does that mean that module state in _json module ¹ is also unnecessary?

Yes, unless I missed something subtle, it is

In any case, the module state is unnecessary here.

Does that mean that module state in _json module is also unnecessary?

Note that some modules were too eagerly converted; though parts of the changes were unneeded, so one can argue that the conversion was churn for little or no benefit. On the other hand, reverting such a change will also be churn for little or no benefit.

PEP-687 tries to avoid the unneeded churn.

Though, one could argue that removing the state would reduce unneeded memory allocations. Such value might be worth the churn.

The same change can be done in many modules, so I'd be much happier if it was discussed more broadly.
Specifically, it looks like the only reason to do it is to satisfy -X showrefcount, Valgrind and similar tools -- as far as I know, there is no ongoing memory leak if youinitialize the module many times, even in a loop with Py_Initialize/Py_Finalize. The extra memory gets allocated only once per process. (Is that right?)
I personally think that making -X showrefcount, Valgrind and similar tools reliable indicators of memory leaks is a good reason for converting to heap types, but given how often we'd use it to justify PRs, I think it should have been explicitly mentioned in PEP 687. Since it wasn't (sorry, I didn't think of -X showrefcount!), we should get project-wide agreement on the validity of the reason before making the change.

For _json:

Though, one could argue that removing the state would reduce unneeded memory allocations. Such value might be worth the churn.

+1, removing unneeded module state from a single module sounds like a good change.

Agreed. Let's move the discussion to Discourse.

Petr:

[...] I think it should have been explicitly mentioned in PEP 687 [...]

Well, it may fall under the unspecified "other reason" part of this sentence; quoting from the PEP:

Static types that do not need module state access, and have no other reason to be converted, should stay static.

Specifically, it looks like the only reason to do it is to satisfy -X showrefcount, Valgrind and similar tools -- as far as I know, there is no ongoing memory leak if youinitialize the module many times, even in a loop with Py_Initialize/Py_Finalize. The extra memory gets allocated only once per process. (Is that right?)

Many static types have been converted to heap types to fix https://bugs.python.org/issue1635741 which is a generalization of this "use case" of "not leaking memory at Python exit".

Many static types have been converted to heap types to fix https://bugs.python.org/issue1635741 which is a generalization of this "use case" of "not leaking memory at Python exit".

There were many similar bug reports:

• https://bugs.python.org/issue6741
• https://bugs.python.org/issue26888
• https://bugs.python.org/issue21387
• https://bugs.python.org/issue32026

Many static types have been converted to heap types to fix https://bugs.python.org/issue1635741 which is a generalization of this "use case" of "not leaking memory at Python exit".

I wrote a summary when this issue was closed: https://mail.python.org/archives/list/python-dev@python.org/thread/E4C6TDNVDPDNNP73HTGHN5W42LGAE22F/

Well, it may fall under the unspecified "other reason" part

It may, but since this reason applies to all types and it wasn't mentioned in the PEP, using it feels disingenuous.

It may, but since this reason applies to all types and it wasn't mentioned in the PEP, using it feels disingenuous.

Fair enough.

It may, but since this reason applies to all types and it wasn't mentioned in the PEP, using it feels disingenuous.\

This issue (which started as focused for _multiprocessing module) and PEP 687 are only about extension modules so all types only means types in the c extension modules excluding the builtin types and types used internally by the interpreter.

"use case" of "not leaking memory at Python exit"

This is a very important issue that should be solved for users who uses embedding interpreter API.
See: https://pybind11.readthedocs.io/en/stable/advanced/embedding.html#interpreter-lifetime

only about extension modules so all types only means types in the c extension modules excluding the builtin types and types used internally by the interpreter.

Right, builtin types get finalized in _PyTypes_FiniTypes. It might be argued that heap types are a better solution than _PyTypes_FiniTypes, but that's another question.

This is a very important issue that should be solved for users who uses embedding interpreter API.

Really? As far as I know, there is no ongoing memory leak if youinitialize the module many times, even in a loop with Py_Initialize/Py_Finalize. The extra memory gets allocated only once per process. (Is that right?)

The problem is way more complicated than that. It's also about correctness.

Between Python 3.8 and Python 3.11, I modified Python finalization to clear more and more Python objects at exit. For example, Python now clears the empty tuple, the empty string singleton, etc. Problem: if an object contains a reference to one of these singletons and this object survive between a Py_Finalize + Py_Initialize cycle, you get... two singletons: old from from the first Py_Initialize, a new one from the second Py_Initialize. But a singleton is supposed to be unique by design: "obj1 is singleton" is not longer true if there are two singletons!

This bug happened in practice: see https://bugs.python.org/issue45691

This complex problem motivated me to add _PyStaticType_Dealloc(). See: https://bugs.python.org/issue46417#msg410808

In older Python versions, it wasn't an issue since singletons survived between Py_Finalize and Py_Initialize(). But as I wrote, it changed in latest Python versions. See Python/pylifecycle.c:

static void
finalize_interp_types(PyInterpreterState *interp)
{
    _PyUnicode_FiniTypes(interp);
    _PySys_Fini(interp);
    _PyExc_Fini(interp);
    _PyAsyncGen_Fini(interp);
    _PyContext_Fini(interp);
    _PyFloat_FiniType(interp);
    _PyLong_FiniTypes(interp);
    _PyThread_FiniType(interp);
    _PyErr_FiniTypes(interp);
    _PyTypes_Fini(interp);
    _PyTypes_FiniTypes(interp);

    // Call _PyUnicode_ClearInterned() before _PyDict_Fini() since it uses
    // a dict internally.
    _PyUnicode_ClearInterned(interp);

    _PyDict_Fini(interp);
    _PyList_Fini(interp);
    _PyTuple_Fini(interp);

    _PySlice_Fini(interp);

    _PyUnicode_Fini(interp);
    _PyFloat_Fini(interp);
}

Well, the code changed multiple times. I made changes, then some got reverted. And for example, Eric Snow recently added _Py_SINGLETON() API, which somehow reverted some of my enhancements, in the hope that PEP 683 "Immortal Objects" got accepted (but it's not accepted yet).

Things changed multiple times, I failed to keep track of these issues. But well, there are complex issues ;-)

Thanks @vstinner for the explanation.

Since Python 3.11 more and more built-in singletons and cached objects are made static to avoid allocation during interpreter startup and that approach allows both subinterpreter support as the objects are static and immortal and provides performance improvement. Victor made some objects per-interpreter and Eric and I made them static.
See #90699 for more details.

See #31616

As Victor explained, things are not as simple and extension module types should ideally be heap types so that each interpreter has its own copy and does not leak memory at exit.

I would like to emphasize that these changes are only for extension modules and those which currently contains static types.
For built-in types and interpreter private types heap types is not the solution and I am against such a change myself.

All I can say is that static types should be avoided in c extension modules for which PEP 687 was written and approved but then here we are discussing the same thing again, heap types vs static types.

All I can say is that static types should be avoided in c extension modules for which PEP 687 was written and approved but then here we are discussing the same thing again, heap types vs static types.

Again, please re-read the PEP. It has a specific scope, and that scope does not immediately apply here, hence the discussion.

Either we as a group come to an agreement that conversion to heap types is the solution here or I would like to see a counter proposal in form of PR which fixes this issue without using private APIs & heap types as I commented above #94382 (comment). Otherwise because of lack of consensus, this issue would have to be escalated to the steering council for decision/ advice.

cc to PEP 687 discussion participants: @gvanrossum @ncoghlan

I have no technical insight to offer (I don't know this part of the interpreter) but it looks to me like tempers are rising. I agree that the participants here ought to come to an agreement but threatening to involve the SC sound premature.

I am not threatening to involve SC, but just saying that if there is no general agreement, we can ask the SC to provide advice/decision.

Erlend's comment seems to indicate to me that this issue does not falls under what PEP 687 is trying to solve. To me this this issue seems like a perfect issue which can be solved under PEP 687 as the conversion to heap types seems the only solution. Also note as the PEP indicates much of the work is already done and most of static types of extensions modules are already ported to heap types.

Don’t waste th SC’s time.

For heap type conversion, PEP-687 explicitly applies to the following modules, as they contain types that access module state:

• Modules/_asynciomodule.c
• Modules/_ctypes/_ctypes.c
• Modules/_datetimemodule.c
• Modules/_decimal/_decimal.c
• Modules/_pickle.c
• Modules/_zoneinfo.c

Maybe I missed a module.

For other types of issues (that is: issues that is not «this module contains static types that access global module state»), PEP-687 may apply (ref. Petr and my comments earlier). In order to find out, a discussion on Discourse (and/or python-dev) is appropriate. Personally, I am fine with this change. Perhaps Petr also is fine with this. But our approvals are not relevant; it does not change the status quo: we must reach a general agreement that PEP-687 applies to these (this issue) types of problems as well.

Remember a the core concept (IMO) of PEP-687 is communicating changes to the whole team.

As mentioned earlier in this thread, an appropriate path forward would be to start a topic on Discourse and ask if it is ok that PEP-687 implicitly can apply to issues like this, as well as the issues explicitly mentioned in the PEP. If there is no general consensus we can ping the SC. Either by asking them for advice, or by submitting another PEP for such issues.

For heap type conversion, PEP-687 explicitly applies to the following modules, as they contain types that access module state:
Modules/_asynciomodule.c
Modules/_ctypes/_ctypes.c
Modules/_datetimemodule.c
Modules/_decimal/_decimal.c
Modules/_pickle.c
Modules/_zoneinfo.c
Maybe I missed a module.

Thanks Erlend for the list but it would have been better if the PEP had explicitly mentioned this list.

If there is no general consensus we can ping the SC. Either by asking them for advice, or by submitting another PEP for such issues.

This is what I was trying to say if there is no general agreement, we can ask the SC for decision/advice.

This is what I was trying to say if there is no general agreement, we can ask the SC for decision/advice

Perhaps there is a misunderstanding about what "general agreement" means. If this group (those involved on this issue) reach an agreement, that is not a "general agreement". A general agreement can only be reached amongst all core devs. Thus, the next level is creating a topic on Discourse, as has been mentioned multiple times already.

@erlend-aasland: I'll create a discourse topic to discuss to this soon.

Also, I apologies incase any of my wording sounded a bit rude but it was not my intension to hurt anyone here.

Hi,
I agree that the change looks good from what I know now, but I don't think I (or any other single person) should make the decision.
As Victor wrote:

Things changed multiple times, I failed to keep track of these issues. But well, there are complex issues ;-)

For complex issues, we have the PEP process. It's not just for getting agreement, but also for untangling/explaining the complexity. And while this change is small enough that a full PEP might not be needed, IMO it does need a clear explanation of what's wrong and why heap types are the best solution. And perhaps we'll not get there with just a discussion thread.
This is not fixing a bug that users are seeing. It's a trade-off between correctness/maintainability and (percieved?) performance. There might be other issues we're not seeing.

(FWIW, I also fail to keep track of these issues, as you can see from the fact that this was not covered in the PEP.)

Discourse topic created here: https://discuss.python.org/t/extension-modules-with-static-types-and-python-embedding-and-subinterpreters/16971

In the topic you still say this is a problem for embedders and multiple interpreters aka subinterpreters which in embedding case will leak memory whenever Python is initialized and finalized one or more times in the same process, while I still think the extra memory gets allocated only once per process. Am I wrong? Could you show a case where there is an unlimited memory leak?

Am I wrong? Could you show a case where there is an unlimited memory leak?

Note that memory leak is much less in Python 3.12 and 3.11 as the runtime finalization even clears static types.
I have seen more complicated cases where gc is involved and if the object survives after its interpreter is freed.

from test import support
import sys
for i in range(1, 10):
    support.run_in_subinterp("import multiprocessing")
    print("refs:", sys.gettotalrefcount(), "blocks: ", sys.getallocatedblocks())

Even if we consider the case for multiple finalization is less important still python leaks memory at exit even if it is initialized only once. Let's continue the discussion on discourse.

Output of the script:

refs: 161895 blocks:  47464
refs: 161958 blocks:  47484
refs: 162016 blocks:  47504
refs: 162074 blocks:  47524
refs: 162132 blocks:  47544
refs: 162190 blocks:  47564
refs: 162248 blocks:  47584
refs: 162306 blocks:  47604
refs: 162364 blocks:  47624

For me, it's a real memory leak. For the specific case of PR #94336, I don't think that we have to involve the SC or write a PEP. I reviewed the PR. IMO if you make requested changes, the PR can be merged.

IMO if you make requested changes, the PR can be merged.

Requested changes have been made and also news entry added.

Kumar, could you please update the PR title and NEWS entry to more accurately describe the change? Apart from that, I'm fine with this.

Thanks for raising the discussion! 🙂

(Sorry, this comment was intended for the PR)

Kumar, could you please update the PR title and NEWS entry to more accurately describe the change? Apart from that, I'm fine with this.

"port multiprocessing static types to heap types" seems fine but I am open to suggestions. Feel free to edit it as you wish :)

Thanks for raising the discussion! 🙂

Thanks! :)

Fixed in main with #94336

Fixed in main with #94336

Nice! One less.