-
-
Notifications
You must be signed in to change notification settings - Fork 30.4k
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.
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
bpo-42972: Fully implement GC protocol for re types #26368
Conversation
@@ -1417,6 +1436,7 @@ _sre_compile_impl(PyObject *module, PyObject *pattern, int flags, | |||
break; | |||
} | |||
} | |||
PyObject_GC_Track(self); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pablogsal I'm unsure if this it the correct place to start tracking the newly created pattern object. My reasoning was that AFAICS, it seems to be fully initialised here, unless I've missed something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO it's the right place.
It must be called before the first DECREF(self), otherwise PyObject_GC_UnTrack() crash in dealloc.
Usually, it's better to only track an object once it's fully initialized, but here it's more convient to do it here to be able to simply call DECREF() on error. The bare minimum is that traverse must be crash when an object started to be tracked. I added a test in debug mode for that: see traverse(visit_validate) in PyObject_GC_Track().
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure that calling PyObject_ClearWeakRefs() in a clear function is safe. What do you think?
@@ -1417,6 +1436,7 @@ _sre_compile_impl(PyObject *module, PyObject *pattern, int flags, | |||
break; | |||
} | |||
} | |||
PyObject_GC_Track(self); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO it's the right place.
It must be called before the first DECREF(self), otherwise PyObject_GC_UnTrack() crash in dealloc.
Usually, it's better to only track an object once it's fully initialized, but here it's more convient to do it here to be able to simply call DECREF() on error. The bare minimum is that traverse must be crash when an object started to be tracked. I added a test in debug mode for that: see traverse(visit_validate) in PyObject_GC_Track().
… refs in tp_clear
Thanks @erlend-aasland for the PR, and @vstinner for merging it 🌮🎉.. I'm working now to backport this PR to: 3.10. |
GH-26411 is a backport of this pull request to the 3.10 branch. |
(cherry picked from commit fba42d1) Co-authored-by: Erlend Egeberg Aasland <erlend.aasland@innova.no>
Thanks @erlend-aasland for the PR, and @vstinner for merging it 🌮🎉.. I'm working now to backport this PR to: 3.10. |
(cherry picked from commit fba42d1) Co-authored-by: Erlend Egeberg Aasland <erlend.aasland@innova.no>
GH-26414 is a backport of this pull request to the 3.10 branch. |
https://bugs.python.org/issue42972