bpo-28416: Break reference cycles in Pickler and Unpickler subclasses #4080
Conversation
with the persistent_id() and persistent_load() methods.
serhiy-storchaka
added
needs backport to 3.6
type-bug
Lib/test/test_pickle.py
Outdated
| @support.cpython_only | ||
| def test_pickler_reference_cycle(self): | ||
| def check(Pickler): | ||
| for bin in 0, 1: |
There was a problem hiding this comment.
Hmm, what is bin? Do you mean protocol?
There was a problem hiding this comment.
A flag text/binary. Later it became a protocol, but the attribute bin still is used in sources (it is equal protocol != 0). Here it is enough to test only one binary protocol.
There was a problem hiding this comment.
Yes... It's still a bit cryptic though. And it doesn't cost anything to loop over all protocols, right? :-)
Modules/_pickle.c
Outdated
| func = _PyObject_GetAttrId(self, name); | ||
| if (func == NULL) { | ||
| *unbound = 0; | ||
| Py_CLEAR(*method); |
There was a problem hiding this comment.
The two lines above seem redundant with the function beginning.
There was a problem hiding this comment.
Actually lines at function beginning are redundant.
Modules/_pickle.c
Outdated
| if (f != NULL) { | ||
| return f(func, self, (PyObject *)Py_TYPE(self)); | ||
| } | ||
| } |
There was a problem hiding this comment.
Why does this fall through? May we return a non-bound method when unbound is true?
There was a problem hiding this comment.
Hmm, interesting question.
Modules/_pickle.c
Outdated
| @@ -360,6 +360,81 @@ _Pickle_FastCall(PyObject *func, PyObject *obj) | |||
|
|
|||
| /*************************************************************************/ | |||
|
|
|||
| static int | |||
There was a problem hiding this comment.
I wonder if the role of those three functions would be clearer if they took a {PyObject *func, int unbound} structure.
There was a problem hiding this comment.
I don't know. This function returns a pair: a pointer to bound or unbound method, and a flag for distinguishing them. There are other functions that return the same pair: _PyObject_GetMethod() and lookup_maybe_method() in typeobject.c. But the interface is different. _PyObject_GetMethod() returns a flag and sets a pointer by reference, lookup_maybe_method() returns a pointer and sets a flag by reference, this functions sets both pointer and flag by reference (and it also decrement the refcount of the old pointer). I tried different variants, but I don't know what interface is better. Well, will try to use a structure.
|
Using a structure will lead to larger diff. Currently the code that just examine But I tried other refactoring. Now an additional parameter is not a flag, but a borrowed reference to self or NULL. This simplifies some calls. |
Modules/_pickle.c
Outdated
| } | ||
| } | ||
|
|
||
| /* Bound a method if it was deconstructed */ |
There was a problem hiding this comment.
Infinitive is "bind", not "bound".
|
Thank you. This looks good to me now. |
|
Thanks @serhiy-storchaka for the PR 🌮🎉.. I'm working now to backport this PR to: 3.6. |
|
GH-4653 is a backport of this pull request to the 3.6 branch. |
…pythonGH-4080) with the persistent_id() and persistent_load() methods. (cherry picked from commit 986375e)
|
Thanks Antoine! |
with the persistent_id() and persistent_load() methods.
https://bugs.python.org/issue28416