gh-157176: Make time.struct_time type immutable

[ashm-dev]

• Issue: Memory leak on interpreter shutdown when reference cycle exists between structseq type and its instance #157176

[ashm-dev]

@sergey-miryanov, could you please review this PR?

[vstinner]

The reported issue is a leak when modifying structseq types. But this PR changes how structseq instances are tracked by the GC. Can you explain me the relationship between the two?

[ashm-dev]

The reported issue is a leak when modifying structseq types. But this PR changes how structseq instances are tracked by the GC. Can you explain me the relationship between the two?

They are the same bug. Modifying the type creates the cycle type -> tp_dict -> t -> type. It leaks because t is untracked, so the GC cannot see the t -> type edge. Tracking the instance makes that edge visible and lets the GC collect the cycle.

[vstinner]

See also #157447 change which is limited to sys.unraisablehook.

[serhiy-storchaka]

Claude initially wrote exactly this change. But it is potentially breaking, the following PyObject_GC_Track() in the user code will crash.

[ashm-dev]

Claude initially wrote exactly this change. But it is potentially breaking, the following PyObject_GC_Track() in the user code will crash.

But the fix in your PR doesn't resolve the leak from my issue.

[sergey-miryanov]

But the fix in your PR doesn't resolve the leak from my issue.

Your both issues have a common root but fix from #157447 shouldn't fix your issue.

[ashm-dev]

Friendly ping @serhiy-storchaka @vstinner — what's the plan here?

[vstinner]

Sadly, changing the public PyStructSequence_New() C API sounds dangerous. As @serhiy-storchaka wrote, it can break existing C extensions which call PyObject_GC_Track().

You can change internal C APIs or modify stdlib extensions to call PyObject_GC_Track() after PyStructSequence_New(). I'm not sure of what's the best approach.

Also, if it's recommended to track objects created by PyStructSequence_New() in the GC, you should mention it in PyStructSequence_New() documentation.

[ashm-dev]

Should I revert the PyStructSequence_New() change, add PyObject_GC_Track() to stdlib callers (time, os, etc.) similar to #157447, and document the tracking requirement in PyStructSequence_New() docs?

[vstinner]

Should I revert the PyStructSequence_New() change, add PyObject_GC_Track() to stdlib callers (time, os, etc.) similar to #157447, and document the tracking requirement in PyStructSequence_New() docs?

Yes, but only add PyObject_GC_Track() if an object can contains other objects which can be involved in a reference cycle.

[ashm-dev]

@vstinner Done. Reverted changes to PyStructSequence_New(), added PyObject_GC_Track() to time.struct_time and sys.get_asyncgen_hooks(), documented the requirement in PyStructSequence_New() docs, and updated tests.

[vstinner]

        # Instances created via Python or stdlib callers that may participate
        # in cycles are GC-tracked; simple instances like os.stat_result are not.
        self.assertTrue(gc.is_tracked(time.gmtime()))

time.gmtime() members are all integers. How can it be involved in a reference cycle?

Code from the issue:

./python -c "import time; t = time.gmtime(); type(t).refcyle = t;"

In your example, you modify the type, not a structseq instance.

I'm not sure that this change is correct.

cc @serhiy-storchaka

[vstinner]

Instead, the type should be made immutable: #157176 (comment).

[serhiy-storchaka]

AFAIK only the argument of sys.unraisablehook contains other objects which can be involved in a reference cycle (this is #157447).

[vstinner]

LGTM. I just have a request on the test.

cc @methane

[vstinner]

The Docs CI failed with:

      File "/home/runner/work/cpython/cpython/Doc/venv/lib/python3.14/site-packages/sphinx/builders/changes.py", line 61, in write_documents
        ttext = self.typemap[changeset.type]
                ~~~~~~~~~~~~^^^^^^^^^^^^^^^^
    KeyError: 'soft-deprecated'

Let me update the branch to see if it does fix the issue.

[vstinner]

I enabled auto-merged, please don't touch the PR (your branch) anymore.

Thanks for support.check_immutable_type(self, type(time.gmtime())). The intent of the test is now more obvious (at least,to me).

[StanFromIreland]

This should be in Library.

[vstinner]

Oh! I didn't pay attention to the section of the Changelog entry. Sadly, I saw your comment after the PR was merged.

[vstinner]

Merged. Thanks for your fix.