Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Sep 1, 2025
Merged

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

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
a8e8501
fully implement GC protocol for `bz2` objects
picnixz Aug 30, 2025
c9d3ab6
remove redundant casts
picnixz Aug 31, 2025
81f77b6
define fields post zero-initialization when needed
picnixz Aug 31, 2025
2b50446
use `PyObject_GC_Del` for clarity
picnixz Aug 31, 2025
a9b6096
reduce diff for 3.14 and later
picnixz Aug 31, 2025
91a3e21
smash diff
picnixz Aug 31, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 15 additions & 8 deletions Modules/_bz2module.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -352,7 +352,7 @@ _bz2_BZ2Compressor_impl(PyTypeObject *type, int compresslevel)
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member Author

@picnixz picnixz Aug 31, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@picnixz picnixz Aug 31, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if (self == NULL) {
return NULL;
}
Expand All @@ -364,13 +364,16 @@ _bz2_BZ2Compressor_impl(PyTypeObject *type, int compresslevel)
return NULL;
}

// explicit fields initialization as PyObject_GC_New() does not change them
self->flushed = 0;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

self->bzs.opaque = NULL;
self->bzs.bzalloc = BZ2_Malloc;
self->bzs.bzfree = BZ2_Free;
bzerror = BZ2_bzCompressInit(&self->bzs, compresslevel, 0, 0);
if (catch_bz2_error(bzerror))
goto error;

PyObject_GC_Track((PyObject *)self);
return (PyObject *)self;

error:
Expand All @@ -381,12 +384,13 @@ _bz2_BZ2Compressor_impl(PyTypeObject *type, int compresslevel)
static void
BZ2Compressor_dealloc(PyObject *op)
{
PyTypeObject *tp = Py_TYPE(op);
PyObject_GC_UnTrack(op);
BZ2Compressor *self = _BZ2Compressor_CAST(op);
BZ2_bzCompressEnd(&self->bzs);
if (self->lock != NULL) {
PyThread_free_lock(self->lock);
}
PyTypeObject *tp = Py_TYPE(self);
tp->tp_free((PyObject *)self);
Py_DECREF(tp);
}
Expand Down Expand Up @@ -420,7 +424,7 @@ static PyType_Spec bz2_compressor_type_spec = {
// bz2_compressor_type_spec does not have Py_TPFLAGS_BASETYPE flag
// which prevents to create a subclass.
// So calling PyType_GetModuleState() in this file is always safe.
.flags = (Py_TPFLAGS_DEFAULT | Py_TPFLAGS_IMMUTABLETYPE),
.flags = (Py_TPFLAGS_DEFAULT | Py_TPFLAGS_IMMUTABLETYPE | Py_TPFLAGS_HAVE_GC),
.slots = bz2_compressor_type_slots,
};

Expand Down Expand Up @@ -653,7 +657,7 @@ _bz2_BZ2Decompressor_impl(PyTypeObject *type)
int bzerror;

assert(type != NULL && type->tp_alloc != NULL);
self = (BZ2Decompressor *)type->tp_alloc(type, 0);
self = PyObject_GC_New(BZ2Decompressor, type);
if (self == NULL) {
return NULL;
}
Expand All @@ -665,6 +669,8 @@ _bz2_BZ2Decompressor_impl(PyTypeObject *type)
return NULL;
}

// explicit fields initialization as PyObject_GC_New() does not change them
self->eof = 0;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

self->needs_input = 1;
self->bzs_avail_in_real = 0;
self->input_buffer = NULL;
Expand All @@ -673,10 +679,12 @@ _bz2_BZ2Decompressor_impl(PyTypeObject *type)
if (self->unused_data == NULL)
goto error;

self->bzs = (bz_stream){.opaque = NULL, .bzalloc = NULL, .bzfree = NULL};
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without this being set, we have a segmentation fault.

bzerror = BZ2_bzDecompressInit(&self->bzs, 0, 0);
if (catch_bz2_error(bzerror))
goto error;

PyObject_GC_Track((PyObject *)self);
return (PyObject *)self;

error:
Expand All @@ -687,8 +695,9 @@ _bz2_BZ2Decompressor_impl(PyTypeObject *type)
static void
BZ2Decompressor_dealloc(PyObject *op)
{
PyTypeObject *tp = Py_TYPE(op);
PyObject_GC_UnTrack(op);
BZ2Decompressor *self = _BZ2Decompressor_CAST(op);

if(self->input_buffer != NULL) {
PyMem_Free(self->input_buffer);
}
Expand All @@ -697,8 +706,6 @@ BZ2Decompressor_dealloc(PyObject *op)
if (self->lock != NULL) {
PyThread_free_lock(self->lock);
}

PyTypeObject *tp = Py_TYPE(self);
tp->tp_free((PyObject *)self);
Py_DECREF(tp);
}
Expand Down Expand Up @@ -751,7 +758,7 @@ static PyType_Spec bz2_decompressor_type_spec = {
// bz2_decompressor_type_spec does not have Py_TPFLAGS_BASETYPE flag
// which prevents to create a subclass.
// So calling PyType_GetModuleState() in this file is always safe.
.flags = (Py_TPFLAGS_DEFAULT | Py_TPFLAGS_IMMUTABLETYPE),
.flags = (Py_TPFLAGS_DEFAULT | Py_TPFLAGS_IMMUTABLETYPE | Py_TPFLAGS_HAVE_GC),
.slots = bz2_decompressor_type_slots,
};

Expand Down
Loading