classification
Title: _Pickler_New() doesn't call PyObject_GC_Track(self)
Type: behavior Stage: resolved
Components: Extension Modules Versions: Python 3.8, Python 3.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: ZackerySpytz, benjamin.peterson, christian.heimes, inada.naoki, miss-islington, serhiy.storchaka, terry.reedy, vstinner
Priority: normal Keywords: patch

Created on 2013-07-05 22:53 by christian.heimes, last changed 2019-04-23 12:25 by inada.naoki. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 8505 merged ZackerySpytz, 2018-07-27 18:53
PR 12926 merged miss-islington, 2019-04-23 11:56
Messages (14)
msg192369 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2013-07-05 22:53
In _Pickler_New() PicklerObject is allocated with 

   self = PyObject_GC_New(PicklerObject, &Pickler_Type);

but PyObject_GC_Track(self) is never called, same for _Unpickler_New(). I think it's a bug.
msg228604 - (view) Author: Mark Lawrence (BreamoreBoy) * Date: 2014-10-05 18:04
@Christian/Victor could either of you provide a patch for this, it's way beyond my knowledge I'm afraid.
msg228610 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2014-10-05 18:25
Mark, could you please stop touching every issue on the tracker? I appreciate the effort, but giving thoughtful feedback, patches, or reviews on a just a few issues would be much more helpful.
msg228613 - (view) Author: Mark Lawrence (BreamoreBoy) * Date: 2014-10-05 18:40
Sorry but no, when I started out on this a couple of months ago there were over 600 issues that nobody had even bothered to reply to.  That number is now down to 369.  I believe that around 200 issues have been closed as a result of my efforts.  Do you have a problem with me showing just how easy it is to get issues resolved?
msg228617 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2014-10-05 19:35
Mark, I also appreciate your efforts, as I hope should be obvious from my prior responses to many of your other posts.  I also agree that somewhat fewer, higher quality posts would be even more helpful.  Please do not slip into a 'confrontational' mode, as you did a few years ago.  And please lets discuss this further by private email.
msg228619 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2014-10-05 21:20
Le 5 oct. 2014 20:04, "Mark Lawrence" <report@bugs.python.org> a écrit :
>
>
> Mark Lawrence added the comment:
>
> @Christian/Victor could either of you provide a patch for this, it's way
beyond my knowledge I'm afraid.

I agree with Benjamin. Asking directly two developers to write a patch
don't add any value to the issue, it only puts pressure on them. If I
didn't write a patch before, it's probably because I'm not really
interested by the topic.

If you would like to contribute, you can close directly issues, test and
review patches, suggest an implementation, etc.

It's not all black or all white. Sometimes, your "ping" messages are
helpful reminders to finish the work or simply to close the issue.

By experience, most old issues are not finished because the bug only impact
a few people and it's possible to work around it, or because the feature
request is not interesting enough.

I didn't read this issue (i'm replying in my mail client), my remarks are
general.
msg228621 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2014-10-05 21:24
I don't know the PyObject_GC_Track() function. Why is it an issue to not call it? Can you elaborate Christian? What do you suggest?
msg228622 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-10-05 21:52
From Doc/c-api/gcsupport.rst:

> Constructors for container types must conform to two rules:
> 
> #. The memory for the object must be allocated using :c:func:`PyObject_GC_New`
>    or :c:func:`PyObject_GC_NewVar`.
> 
> #. Once all the fields which may contain references to other containers are
> 
>    initialized, it must call :c:func:`PyObject_GC_Track`.

_pickle.Pickler and _pickle.Unpickler have the Py_TPFLAGS_HAVE_GC flag, implement tp_traverse and tp_clear, but PyObject_GC_Track is newer called.
msg339604 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-04-08 10:21
It has been decided to not fix the issue in Python 2.7:
https://github.com/python/cpython/pull/8505#issuecomment-480771689
msg339642 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2019-04-08 14:19
Tracking objects that do not need this will just add work to the garbage collector. Not all instances of trackable types should be tracked, for example the empty tuple and some dicts are not tracked.

>>> gc.is_tracked(())
False
>>> gc.is_tracked((1, 2))
True
>>> gc.is_tracked({1: None})
False
>>> gc.is_tracked({1: []})
True
msg339724 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-04-09 10:36
> Tracking objects that do not need this will just add work to the garbage collector. Not all instances of trackable types should be tracked, for example the empty tuple and some dicts are not tracked.

Well, in that case, we should do the opposite of PR 8505, what I proposed there:

https://github.com/python/cpython/pull/8505#issuecomment-480763122

"Either GC support must be removed (remove Py_TPFLAGS_HAVE_GC, remove tp_clear and tp_traverse, etc.), or the implementation should be fixed (call PyObject_GC_Track)."

=> fully remove the GC support

I don't see the point of implementing tp_traverse if it's not called.

I'm not sure if tp_clear is related to the GC or not. Maybe keep it :-)
msg339726 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2019-04-09 10:45
GC support can not be removed, because these objects are created also as long living objects using the standard way. And in this case they are tracked (in PyType_GenericAlloc()) and tp_traverse is called. They are not tracked only when created by an internal API _Pickler_New() and _Unpickler_New().

The current code is correct and I do not see reasons to change it.
msg340718 - (view) Author: Inada Naoki (inada.naoki) * (Python committer) Date: 2019-04-23 11:56
New changeset 359bd4f61b9e1493081f4f67882554247b53926a by Inada Naoki (Zackery Spytz) in branch 'master':
bpo-18372: Add missing PyObject_GC_Track() calls in the pickle module (GH-8505)
https://github.com/python/cpython/commit/359bd4f61b9e1493081f4f67882554247b53926a
msg340719 - (view) Author: miss-islington (miss-islington) Date: 2019-04-23 12:18
New changeset c0f6f5370325459cadd90010530b1d300dce514e by Miss Islington (bot) in branch '3.7':
bpo-18372: Add missing PyObject_GC_Track() calls in the pickle module (GH-8505)
https://github.com/python/cpython/commit/c0f6f5370325459cadd90010530b1d300dce514e
History
Date User Action Args
2019-04-23 12:25:18inada.naokisetstatus: open -> closed
resolution: fixed
stage: patch review -> resolved
2019-04-23 12:18:28miss-islingtonsetnosy: + miss-islington
messages: + msg340719
2019-04-23 11:56:32miss-islingtonsetpull_requests: + pull_request12853
2019-04-23 11:56:11inada.naokisetnosy: + inada.naoki
messages: + msg340718
2019-04-09 10:45:01serhiy.storchakasetmessages: + msg339726
2019-04-09 10:36:22vstinnersetmessages: + msg339724
2019-04-08 14:19:53serhiy.storchakasetmessages: + msg339642
2019-04-08 10:21:32vstinnersetmessages: + msg339604
versions: - Python 3.6
2018-07-28 11:53:40BreamoreBoysetnosy: - BreamoreBoy
2018-07-27 18:54:44ZackerySpytzsetnosy: + ZackerySpytz

versions: + Python 3.6, Python 3.7, Python 3.8, - Python 2.7, Python 3.4, Python 3.5
2018-07-27 18:53:11ZackerySpytzsetkeywords: + patch
stage: needs patch -> patch review
pull_requests: + pull_request8024
2014-10-05 21:52:00serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg228622
2014-10-05 21:24:08vstinnersetmessages: + msg228621
2014-10-05 21:20:23vstinnersetmessages: + msg228619
2014-10-05 19:35:55terry.reedysetnosy: + terry.reedy
messages: + msg228617
2014-10-05 18:40:20BreamoreBoysetmessages: + msg228613
2014-10-05 18:25:28benjamin.petersonsetnosy: + benjamin.peterson
messages: + msg228610
2014-10-05 18:04:43BreamoreBoysetnosy: + BreamoreBoy

messages: + msg228604
versions: + Python 2.7, Python 3.4, Python 3.5
2013-07-12 18:45:54vstinnersetnosy: + vstinner
2013-07-05 22:53:10christian.heimescreate