[Python-checkins] bpo-39492: Fix a reference cycle between reducer_override and a Pickler instance (GH-18266)

[Pierre Glaser]

https://github.com/python/cpython/commit/0f2f35e15f9fbee44ce042b724348419d8136bc5
commit: 0f2f35e15f9fbee44ce042b724348419d8136bc5
branch: master
author: Pierre Glaser <pierreglaser at msn.com>
committer: GitHub <noreply at github.com>
date: 2020-02-02T10:55:21-08:00
summary:

bpo-39492: Fix a reference cycle between reducer_override and a Pickler instance (GH-18266)



This also needs a backport to 3.8


https://bugs.python.org/issue39492



Automerge-Triggered-By: @pitrou

files:
A Misc/NEWS.d/next/Core and Builtins/2020-01-30-01-14-42.bpo-39492.eTuy0F.rst
M Lib/test/pickletester.py
M Modules/_pickle.c

diff --git a/Lib/test/pickletester.py b/Lib/test/pickletester.py
index 953fd5c5a278b..ba893f39c2f34 100644
--- a/Lib/test/pickletester.py
+++ b/Lib/test/pickletester.py
@@ -3499,6 +3499,30 @@ class MyClass:
                         ValueError, 'The reducer just failed'):
                     p.dump(h)
 
+    @support.cpython_only
+    def test_reducer_override_no_reference_cycle(self):
+        # bpo-39492: reducer_override used to induce a spurious reference cycle
+        # inside the Pickler object, that could prevent all serialized objects
+        # from being garbage-collected without explicity invoking gc.collect.
+
+        for proto in range(0, pickle.HIGHEST_PROTOCOL + 1):
+            with self.subTest(proto=proto):
+                def f():
+                    pass
+
+                wr = weakref.ref(f)
+
+                bio = io.BytesIO()
+                p = self.pickler_class(bio, proto)
+                p.dump(f)
+                new_f = pickle.loads(bio.getvalue())
+                assert new_f == 5
+
+                del p
+                del f
+
+                self.assertIsNone(wr())
+
 
 class AbstractDispatchTableTests(unittest.TestCase):
 
diff --git a/Misc/NEWS.d/next/Core and Builtins/2020-01-30-01-14-42.bpo-39492.eTuy0F.rst b/Misc/NEWS.d/next/Core and Builtins/2020-01-30-01-14-42.bpo-39492.eTuy0F.rst
new file mode 100644
index 0000000000000..6e8b715c46365
--- /dev/null
+++ b/Misc/NEWS.d/next/Core and Builtins/2020-01-30-01-14-42.bpo-39492.eTuy0F.rst	
@@ -0,0 +1 @@
+Fix a reference cycle in the C Pickler that was preventing the garbage collection of deleted, pickled objects.
\ No newline at end of file
diff --git a/Modules/_pickle.c b/Modules/_pickle.c
index 25d5c8da92068..fc48d6057866a 100644
--- a/Modules/_pickle.c
+++ b/Modules/_pickle.c
@@ -4455,12 +4455,13 @@ static int
 dump(PicklerObject *self, PyObject *obj)
 {
     const char stop_op = STOP;
+    int status = -1;
     PyObject *tmp;
     _Py_IDENTIFIER(reducer_override);
 
     if (_PyObject_LookupAttrId((PyObject *)self, &PyId_reducer_override,
                                &tmp) < 0) {
-        return -1;
+      goto error;
     }
     /* Cache the reducer_override method, if it exists. */
     if (tmp != NULL) {
@@ -4477,7 +4478,7 @@ dump(PicklerObject *self, PyObject *obj)
         assert(self->proto >= 0 && self->proto < 256);
         header[1] = (unsigned char)self->proto;
         if (_Pickler_Write(self, header, 2) < 0)
-            return -1;
+            goto error;
         if (self->proto >= 4)
             self->framing = 1;
     }
@@ -4485,9 +4486,22 @@ dump(PicklerObject *self, PyObject *obj)
     if (save(self, obj, 0) < 0 ||
         _Pickler_Write(self, &stop_op, 1) < 0 ||
         _Pickler_CommitFrame(self) < 0)
-        return -1;
+        goto error;
+
+    // Success
+    status = 0;
+
+  error:
     self->framing = 0;
-    return 0;
+
+    /* Break the reference cycle we generated at the beginning this function
+     * call when setting the reducer_override attribute of the Pickler instance
+     * to a bound method of the same instance. This is important as the Pickler
+     * instance holds a reference to each object it has pickled (through its
+     * memo): thus, these objects wont be garbage-collected as long as the
+     * Pickler itself is not collected. */
+    Py_CLEAR(self->reducer_override);
+    return status;
 }
 
 /*[clinic input]