From e6e1fca59a44bfa026478aaab36adac9e9b5fc7e Mon Sep 17 00:00:00 2001 From: Pierre Glaser Date: Thu, 30 Jan 2020 02:05:18 +0100 Subject: [PATCH 1/4] break a reference cycle in the C Pickler --- Modules/_pickle.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/Modules/_pickle.c b/Modules/_pickle.c index 25d5c8da920686f..bfb307f6c74c447 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 = 0; 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,15 @@ 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; + if (0) { + error: + status = -1; + } + self->framing = 0; - return 0; + Py_CLEAR(self->reducer_override); + return status; } /*[clinic input] From 9b2bc39e80c1037464ee8beb998ea9326b8b8d61 Mon Sep 17 00:00:00 2001 From: Pierre Glaser Date: Thu, 30 Jan 2020 02:06:57 +0100 Subject: [PATCH 2/4] test that stale pickled object are well gc'ed --- Lib/test/pickletester.py | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/Lib/test/pickletester.py b/Lib/test/pickletester.py index 953fd5c5a278ba4..e4683c271ae683f 100644 --- a/Lib/test/pickletester.py +++ b/Lib/test/pickletester.py @@ -3499,6 +3499,37 @@ class MyClass: ValueError, 'The reducer just failed'): p.dump(h) + 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 + + collect = threading.Event() + _ = weakref.ref(f, lambda obj: collect.set()) + + 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 + + # There is no reference to f anymore. If p was gc'd, so should + # be f. + for i in range(10): + collected = collect.wait(timeout=0.1) + if collected: + break + + self.assertTrue(collected) + class AbstractDispatchTableTests(unittest.TestCase): From eaf3e550da8e89173f8e6a82b628d3697985bd21 Mon Sep 17 00:00:00 2001 From: "blurb-it[bot]" <43283697+blurb-it[bot]@users.noreply.github.com> Date: Thu, 30 Jan 2020 01:14:45 +0000 Subject: [PATCH 3/4] =?UTF-8?q?=F0=9F=93=9C=F0=9F=A4=96=20Added=20by=20blu?= =?UTF-8?q?rb=5Fit.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../Core and Builtins/2020-01-30-01-14-42.bpo-39492.eTuy0F.rst | 1 + 1 file changed, 1 insertion(+) create mode 100644 Misc/NEWS.d/next/Core and Builtins/2020-01-30-01-14-42.bpo-39492.eTuy0F.rst 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 000000000000000..6e8b715c46365b6 --- /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 From d526584369bf7f95ffb7bcd07b769fb4486df1c9 Mon Sep 17 00:00:00 2001 From: Pierre Glaser Date: Sun, 2 Feb 2020 19:27:13 +0100 Subject: [PATCH 4/4] simplifications, comments --- Lib/test/pickletester.py | 13 +++---------- Modules/_pickle.c | 17 ++++++++++++----- 2 files changed, 15 insertions(+), 15 deletions(-) diff --git a/Lib/test/pickletester.py b/Lib/test/pickletester.py index e4683c271ae683f..ba893f39c2f34d7 100644 --- a/Lib/test/pickletester.py +++ b/Lib/test/pickletester.py @@ -3499,6 +3499,7 @@ 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 @@ -3509,8 +3510,7 @@ def test_reducer_override_no_reference_cycle(self): def f(): pass - collect = threading.Event() - _ = weakref.ref(f, lambda obj: collect.set()) + wr = weakref.ref(f) bio = io.BytesIO() p = self.pickler_class(bio, proto) @@ -3521,14 +3521,7 @@ def f(): del p del f - # There is no reference to f anymore. If p was gc'd, so should - # be f. - for i in range(10): - collected = collect.wait(timeout=0.1) - if collected: - break - - self.assertTrue(collected) + self.assertIsNone(wr()) class AbstractDispatchTableTests(unittest.TestCase): diff --git a/Modules/_pickle.c b/Modules/_pickle.c index bfb307f6c74c447..fc48d6057866a91 100644 --- a/Modules/_pickle.c +++ b/Modules/_pickle.c @@ -4455,7 +4455,7 @@ static int dump(PicklerObject *self, PyObject *obj) { const char stop_op = STOP; - int status = 0; + int status = -1; PyObject *tmp; _Py_IDENTIFIER(reducer_override); @@ -4487,12 +4487,19 @@ dump(PicklerObject *self, PyObject *obj) _Pickler_Write(self, &stop_op, 1) < 0 || _Pickler_CommitFrame(self) < 0) goto error; - if (0) { - error: - status = -1; - } + // Success + status = 0; + + error: self->framing = 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; }