gh-116946: fully implement GC protocol for `bz2` objects #138266

picnixz · 2025-08-30T14:26:40Z

AFAIU, tp_traverse will not be called if Py_TPFLAGS_HAVE_GC is not specified. Quoting the specs:

Instances of heap-allocated types hold a reference to their type. Their traversal function must therefore either visit Py_TYPE(self), or delegate this responsibility by calling tp_traverse of another heap-allocated type (such as a heap-allocated superclass). If they do not, the type object may not be garbage-collected.

As such, I believe that this is the correct fix (that is, we need tp_traverse to be called).

cc @vstinner

Issue: Heap types implemented in C must have Py_TPFLAGS_HAVE_GC and implement tp_traverse #116946

picnixz · 2025-08-30T14:45:49Z

Modules/_bz2module.c

    }

+    // explicit fields initialization as PyObject_GC_New() does not change them
+    self->flushed = 0;


We need this to be explicitly initialized otherwise tests fail because flushed would be arbitrary.

picnixz · 2025-08-30T14:46:04Z

Modules/_bz2module.c

    if (self->unused_data == NULL)
        goto error;

+    self->bzs = (bz_stream){.opaque = NULL, .bzalloc = NULL, .bzfree = NULL};


Without this being set, we have a segmentation fault.

picnixz · 2025-08-30T14:46:39Z

Modules/_bz2module.c

    }

+    // explicit fields initialization as PyObject_GC_New() does not change them
+    self->eof = 0;


Without this being set by default, tests fail as eof is most likely "true" by default (that's because of the call to PyObject_GC_New which initializes the memory but doesn't zero it).

bedevere-bot · 2025-08-30T14:46:59Z

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

Results will be shown at:

https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F138266%2Fmerge

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

ZeroIntensity · 2025-08-30T15:20:58Z

Modules/_bz2module.c


    assert(type != NULL && type->tp_alloc != NULL);
-    self = (BZ2Compressor *)type->tp_alloc(type, 0);
+    self = PyObject_GC_New(BZ2Compressor, type);


Hm, this seems like introducing additional maintenance for no reason. If a type has Py_TPFLAGS_HAVE_GC set, then tp_alloc will act like PyObject_GC_New anyway, except the data will also be zero-ed (so we wouldn't need the additional change below).

I don't like those kind of "will act like" [...]. I prefer using the public API correctly rather than relying on implementation details. I know it's an additional maintenance burden but I think it's worth it because we would follow our own rules.

Also, I think we should perhaps add a function for zero data with PyObject_GC_New to prevent these additional changes while using public functions (and not just accessing tp_alloc directly).

I get what you're saying, but this really isn't an implementation detail. The docs explicitly tell users to prefer tp_alloc instead of manually allocating objects. The only place where I'd say it's ok to use PyObject_GC_New is when using the limited API.

Then we're not reading the same docs: https://docs.python.org/3/c-api/gcsupport.html#supporting-cycle-detection

Constructors for container types must conform to two rules:
The memory for the object must be allocated using PyObject_GC_New or PyObject_GC_NewVar.

And https://docs.python.org/3/c-api/typeobj.html#c.Py_TPFLAGS_HEAPTYPE:

Heap types should also support garbage collection as they can form a reference cycle with their own module object

and in https://docs.python.org/3/c-api/typeobj.html#c.PyTypeObject.tp_traverse:

More information about Python’s garbage collection scheme can be found in section Supporting Cyclic Garbage Collection.

All those documented area point to the page where we explicitly ask users to use PyObject_GC_New[Var]. I haven't seen anywhere something recommending tp_alloc. And https://docs.python.org/3/c-api/type.html#c.PyType_GenericAlloc doesn't document that this can be used instead of GC_New + GC_Track.

Yeah, this was part of the big object lifecycle documentation update in 3.14: #125962

Modules/_bz2module.c

ZeroIntensity

Sorry to block like this, but I feel strongly -1 on the switch from tp_alloc to PyObject_GC_New. In 3.14 and 3.15, all of the C API documentation asks that you choose tp_alloc over manual object allocation functions.

ZeroIntensity · 2025-08-31T11:58:30Z

Modules/_bz2module.c


    assert(type != NULL && type->tp_alloc != NULL);
-    self = (BZ2Compressor *)type->tp_alloc(type, 0);
+    self = PyObject_GC_New(BZ2Compressor, type);


I get what you're saying, but this really isn't an implementation detail. The docs explicitly tell users to prefer tp_alloc instead of manually allocating objects. The only place where I'd say it's ok to use PyObject_GC_New is when using the limited API.

bedevere-app · 2025-08-31T12:00:08Z

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

picnixz · 2025-08-31T12:11:32Z

In 3.14 and 3.15, all of the C API documentation asks that you choose tp_alloc over manual object allocation functions.

Ah that's why. The stable docs being 3.13, this wasn't indicated! I can switch to do this then, though I really hate invoking directly the slot function.

picnixz · 2025-08-31T12:13:26Z

I'll update the other PRs for 3.14+, that I can do. However, does the behavior also hold in 3.13? or is a 3.14+ feature?

ZeroIntensity · 2025-08-31T12:17:17Z

The behavior holds for 3.13, we just changed the preference in the docs in 3.14.

though I really hate invoking directly the slot function.

How come? I think it's just nicer to use. If you change the type from GC to non-GC, for example, you don't have to update all your tp_alloc and tp_free calls.

ZeroIntensity · 2025-08-31T12:23:56Z

Modules/_bz2module.c

-
-    PyTypeObject *tp = Py_TYPE(self);
-    tp->tp_free((PyObject *)self);
+    PyObject_GC_Del(self);


Please also prefer tp_free over PyObject_GC_Del. The new docs say that too:

Do not call this directly to free an object’s memory; call the type’s tp_free slot instead.

Yeah I just swa that as well, so I'm going to add another commit (I'm modifying the other 3 PRs at the same time)

picnixz · 2025-08-31T12:27:32Z

How come? I think it's just nicer to use. If you change the type from GC to non-GC, for example, you don't have to update all your tp_alloc and tp_free calls.

I would have preferred invoking a consumer function that is, PyObject_GenericFree(op) so that I don't need to use tp->tp_free(op). It looks nicer to me, but those are really style preferences.

ZeroIntensity · 2025-08-31T12:29:41Z

I'd be okay with a PyType_InvokeAlloc or PyType_InvokeFree on the basis that it would make things easier for limited API users, but that's out of the scope of these PRs.

picnixz · 2025-08-31T12:36:55Z

Hum, now it feels wrong but is it ok that the GC is tracking the object when I'm not done yet with setting its fields? because it appears that now alloc functions automatically track the object after the call and this violates the recommendations for https://docs.python.org/3.14/c-api/gcsupport.html#c.PyObject_GC_Track.

ZeroIntensity · 2025-08-31T12:39:11Z

Yeah, that's fine. In practice, most object won't call into Python code before they're fully initialized, and even if one does, that's why there's a big red note at gc.get_referrers.

picnixz · 2025-08-31T12:43:03Z

Yeah, but this is still wrong. What if someone is calling some method on the argument? that would call into Python code. I think we should also have a function for doing the alloc but not tracking the object yet. For now, I'll live with this but I really think this should be documented at the level of tp_alloc as well (it doesn't say that it tracks the object...)

ZeroIntensity · 2025-08-31T12:46:48Z

Yeah, but this is still wrong. What if someone is calling some method on the argument?

Unless they're playing with gc.get_referrers, nothing should happen, right? The GC won't crash because the fields are correctly zeroed out by tp_alloc.

documented at the level of tp_alloc as well (it doesn't say that it tracks the object...)

Sure, do you want to make the PR?

picnixz · 2025-08-31T13:04:20Z

I have made the requested changes; please review again.

Unless they're playing with gc.get_referrers, nothing should happen, right

I hope you're right. OTOH, users could first construct the fields separately and set them afterwards (though they won't be able to rely on Py_DECREF(self) to release fields that were successfully created).

Sure, do you want to make the PR?

I don't mind but I think it's also an easy contribution for newcomers.

picnixz · 2025-08-31T13:08:05Z

Thanks for humoring me with tp_alloc, this looks good.

I think we should still backport the recommendations to 3.13 if nothing changed on our side (if this was always the behavior of tp_alloc & co, then it's probably worth doing this). However, we should still update https://docs.python.org/3.14/c-api/gcsupport.html#supporting-cycle-detection because it says "Use GC_New" and under "GC_New" it says "use tp_alloc" and this would be confusing.

picnixz · 2025-08-31T13:09:08Z

I'll create a doc issue for that because there are two tasks:

Indicate that tp_alloc may call PyObject_GC_Track.
Reword https://docs.python.org/3/c-api/gcsupport.html#supporting-cycle-detection.

ZeroIntensity · 2025-08-31T13:11:26Z

I think we should still backport the recommendations to 3.13 if nothing changed on our side (if this was always the behavior of tp_alloc & co, then it's probably worth doing this)

Sounds good, but up to you on whether to put in the effort, because 3.14's documentation will be the default in a month anyway.

picnixz · 2025-08-31T13:20:13Z

Mmmh. Right. No need for the docs backports.

AA-Turner · 2025-08-31T14:06:13Z

Off-topic, but I generally default to reading https://docs.python.org/dev/ -- this is the version that reflects HEAD, whereas /3/ reflects what downstream users should be using.

A

miss-islington-app · 2025-09-01T08:22:32Z

Thanks @picnixz for the PR 🌮🎉.. I'm working now to backport this PR to: 3.13.
🐍🍒⛏🤖

miss-islington-app · 2025-09-01T08:22:32Z

Thanks @picnixz for the PR 🌮🎉.. I'm working now to backport this PR to: 3.14.
🐍🍒⛏🤖

miss-islington-app · 2025-09-01T08:22:35Z

Sorry, @picnixz, I could not cleanly backport this to 3.13 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 9be91f6a20ed2fd9b491c3e779dc45c7392f60ca 3.13

…nGH-138266) (cherry picked from commit 9be91f6) Co-authored-by: Bénédikt Tran <[email protected]>

bedevere-app · 2025-09-01T08:22:52Z

GH-138320 is a backport of this pull request to the 3.14 branch.

…pythonGH-138266) (cherry picked from commit 9be91f6) Co-authored-by: Bénédikt Tran <[email protected]>

bedevere-app · 2025-09-01T08:29:35Z

GH-138322 is a backport of this pull request to the 3.13 branch.

…38266) (#138322) (cherry picked from commit 9be91f6)

… objects (pythonGH-138266) (python#138322)" This reverts commit 90036f5.

python#138266)" This reverts commit 9be91f6.

…H-138322, GH-138323, GH-138326) (#138337) * Revert "[3.13] gh-116946: fully implement GC protocol for `bz2` objects (GH-138266) (#138322)" This reverts commit 90036f5. * Revert "[3.13] gh-116946: fully implement GC protocol for `lzma` objects (GH-138288) (#138323)" This reverts commit 828682d. * Revert "[3.13] gh-116946: fully implement GC protocol for `_hashlib` objects (GH-138289) (#138326)" This reverts commit 21b5932.

…, GH-138288, GH-138289) (#138338) * Revert "gh-116946: fully implement GC protocol for `bz2` objects (#138266)" This reverts commit 9be91f6. * Revert "gh-116946: fully implement GC protocol for `lzma` objects (#138288)" This reverts commit 3ea16f9. * Revert "gh-116946: fully implement GC protocol for `_hashlib` objects (#138289)" This reverts commit 6f1dd95.

…n#138266)

…ythonGH-138266, pythonGH-138288, pythonGH-138289) (python#138338) * Revert "pythongh-116946: fully implement GC protocol for `bz2` objects (python#138266)" This reverts commit 9be91f6. * Revert "pythongh-116946: fully implement GC protocol for `lzma` objects (python#138288)" This reverts commit 3ea16f9. * Revert "pythongh-116946: fully implement GC protocol for `_hashlib` objects (python#138289)" This reverts commit 6f1dd95.

fully implement GC protocol for bz2 objects

a8e8501

picnixz added the skip news label Aug 30, 2025

bedevere-app bot added the awaiting core review label Aug 30, 2025

bedevere-app bot mentioned this pull request Aug 30, 2025

Heap types implemented in C must have Py_TPFLAGS_HAVE_GC and implement tp_traverse #116946

Open

picnixz commented Aug 30, 2025

View reviewed changes

picnixz added the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Aug 30, 2025

bedevere-bot removed the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Aug 30, 2025

ZeroIntensity reviewed Aug 30, 2025

View reviewed changes

picnixz commented Aug 31, 2025

View reviewed changes

Modules/_bz2module.c Outdated Show resolved Hide resolved

Modules/_bz2module.c Outdated Show resolved Hide resolved

picnixz added 3 commits August 31, 2025 11:16

remove redundant casts

c9d3ab6

define fields post zero-initialization when needed

81f77b6

use PyObject_GC_Del for clarity

2b50446

ZeroIntensity requested changes Aug 31, 2025

View reviewed changes

bedevere-app bot added awaiting changes and removed awaiting core review labels Aug 31, 2025

reduce diff for 3.14 and later

a9b6096

ZeroIntensity reviewed Aug 31, 2025

View reviewed changes

smash diff

91a3e21

bedevere-app bot added awaiting merge and removed awaiting change review labels Aug 31, 2025

picnixz mentioned this pull request Aug 31, 2025

Improve guidelines for GC protocol implementation for heap types #138292

Open

picnixz merged commit 9be91f6 into python:main Sep 1, 2025
48 of 51 checks passed

picnixz deleted the fix/gc/bz2-heap-types-116946 branch September 1, 2025 08:21

bedevere-app bot removed the awaiting merge label Sep 1, 2025

picnixz added needs backport to 3.13 bugs and security fixes needs backport to 3.14 bugs and security fixes labels Sep 1, 2025

miss-islington-app bot assigned picnixz Sep 1, 2025

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Sep 1, 2025

pythongh-116946: fully implement GC protocol for bz2 objects (pytho…

ec564e1

…nGH-138266) (cherry picked from commit 9be91f6) Co-authored-by: Bénédikt Tran <[email protected]>

bedevere-app bot removed the needs backport to 3.14 bugs and security fixes label Sep 1, 2025

picnixz added a commit to picnixz/cpython that referenced this pull request Sep 1, 2025

[3.13] pythongh-116946: fully implement GC protocol for bz2 objects (…

839b0f3

…pythonGH-138266) (cherry picked from commit 9be91f6) Co-authored-by: Bénédikt Tran <[email protected]>

bedevere-app bot removed the needs backport to 3.13 bugs and security fixes label Sep 1, 2025

picnixz added a commit that referenced this pull request Sep 1, 2025

[3.13] gh-116946: fully implement GC protocol for bz2 objects (GH-1…

90036f5

…38266) (#138322) (cherry picked from commit 9be91f6)

picnixz added a commit to picnixz/cpython that referenced this pull request Sep 1, 2025

Revert "[3.13] pythongh-116946: fully implement GC protocol for bz2…

50e1b6e

… objects (pythonGH-138266) (python#138322)" This reverts commit 90036f5.

picnixz added a commit to picnixz/cpython that referenced this pull request Sep 1, 2025

Revert "pythongh-116946: fully implement GC protocol for bz2 objects (

131bab6

python#138266)" This reverts commit 9be91f6.

lkollar pushed a commit to lkollar/cpython that referenced this pull request Sep 9, 2025

pythongh-116946: fully implement GC protocol for bz2 objects (pytho…

90fc2a1

…n#138266)

Uh oh!

gh-116946: fully implement GC protocol for bz2 objects #138266

gh-116946: fully implement GC protocol for bz2 objects #138266

Uh oh!

Conversation

picnixz commented Aug 30, 2025 • edited by bedevere-app bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Aug 30, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

picnixz Aug 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

picnixz Aug 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ZeroIntensity left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bedevere-app bot commented Aug 31, 2025

Uh oh!

picnixz commented Aug 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

picnixz commented Aug 31, 2025

Uh oh!

ZeroIntensity commented Aug 31, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

picnixz commented Aug 31, 2025

Uh oh!

ZeroIntensity commented Aug 31, 2025

Uh oh!

picnixz commented Aug 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ZeroIntensity commented Aug 31, 2025

Uh oh!

picnixz commented Aug 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ZeroIntensity commented Aug 31, 2025

Uh oh!

picnixz commented Aug 31, 2025

Uh oh!

picnixz commented Aug 31, 2025

Uh oh!

picnixz commented Aug 31, 2025

Uh oh!

ZeroIntensity commented Aug 31, 2025

Uh oh!

picnixz commented Aug 31, 2025

Uh oh!

AA-Turner commented Aug 31, 2025

Uh oh!

Uh oh!

miss-islington-app bot commented Sep 1, 2025

Uh oh!

miss-islington-app bot commented Sep 1, 2025

gh-116946: fully implement GC protocol for `bz2` objects #138266

gh-116946: fully implement GC protocol for `bz2` objects #138266

picnixz commented Aug 30, 2025 •

edited by bedevere-app bot

Loading

picnixz Aug 31, 2025 •

edited

Loading

picnixz Aug 31, 2025 •

edited

Loading

picnixz commented Aug 31, 2025 •

edited

Loading

picnixz commented Aug 31, 2025 •

edited

Loading

picnixz commented Aug 31, 2025 •

edited

Loading