gh-116946: fully implement GC protocol for bz2 objects
#138266
Conversation
| @@ -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; | |||
There was a problem hiding this comment.
We need this to be explicitly initialized otherwise tests fail because flushed would be arbitrary.
| @@ -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}; | |||
There was a problem hiding this comment.
Without this being set, we have a segmentation fault.
| @@ -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; | |||
There was a problem hiding this comment.
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).
picnixz
added
the
🔨 test-with-refleak-buildbots
|
🤖 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. |
bedevere-bot
removed
the
🔨 test-with-refleak-buildbots
| @@ -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); | |||
There was a problem hiding this comment.
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).
AFAIU,
tp_traversewill not be called ifPy_TPFLAGS_HAVE_GCis not specified. Quoting the specs:As such, I believe that this is the correct fix (that is, we need
tp_traverseto be called).cc @vstinner