From 6f7abb38d5d284708d47428840b1d05cc0364c0b Mon Sep 17 00:00:00 2001 From: Brandt Bucher Date: Wed, 2 Dec 2020 20:25:49 -0800 Subject: [PATCH 01/23] Add failing regression test. --- Lib/test/test_builtin.py | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/Lib/test/test_builtin.py b/Lib/test/test_builtin.py index edb4ec092e3586..2490a038abc711 100644 --- a/Lib/test/test_builtin.py +++ b/Lib/test/test_builtin.py @@ -6,6 +6,7 @@ import collections import decimal import fractions +import gc import io import locale import os @@ -1756,6 +1757,15 @@ def __next__(self): l8 = self.iter_error(zip(Iter(3), "AB", strict=True), ValueError) self.assertEqual(l8, [(2, "A"), (1, "B")]) + def test_zip_result_gc(self): + try: + gc.disable() + z = zip([[]]) + gc.collect() + self.assertTrue(gc.is_tracked(next(z))) + finally: + gc.enable() + def test_format(self): # Test the basic machinery of the format() builtin. Don't test # the specifics of the various formatters From 5c7b815f8ead387277ad2591c6db2893c57fd97e Mon Sep 17 00:00:00 2001 From: Brandt Bucher Date: Wed, 2 Dec 2020 20:26:40 -0800 Subject: [PATCH 02/23] Re-track recyled result tuples. --- Python/bltinmodule.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/Python/bltinmodule.c b/Python/bltinmodule.c index a73b8cb320e975..0691cb417bd442 100644 --- a/Python/bltinmodule.c +++ b/Python/bltinmodule.c @@ -2636,6 +2636,11 @@ zip_next(zipobject *lz) PyTuple_SET_ITEM(result, i, item); Py_DECREF(olditem); } + // The GC may have untracked this result tuple if its elements were all + // untracked. Since we're recycling it, make sure it's tracked again: + if (!_PyObject_GC_IS_TRACKED(result)) { + _PyObject_GC_TRACK(result); + } } else { result = PyTuple_New(tuplesize); if (result == NULL) From c8f161a00bd214d98a8b10b76c6e75368f8c0b07 Mon Sep 17 00:00:00 2001 From: Brandt Bucher Date: Wed, 2 Dec 2020 20:26:53 -0800 Subject: [PATCH 03/23] Blurb add. --- .../Core and Builtins/2020-12-02-20-23-31.bpo-42536.Kx3ZOu.rst | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 Misc/NEWS.d/next/Core and Builtins/2020-12-02-20-23-31.bpo-42536.Kx3ZOu.rst diff --git a/Misc/NEWS.d/next/Core and Builtins/2020-12-02-20-23-31.bpo-42536.Kx3ZOu.rst b/Misc/NEWS.d/next/Core and Builtins/2020-12-02-20-23-31.bpo-42536.Kx3ZOu.rst new file mode 100644 index 00000000000000..ede2044f6d04f0 --- /dev/null +++ b/Misc/NEWS.d/next/Core and Builtins/2020-12-02-20-23-31.bpo-42536.Kx3ZOu.rst @@ -0,0 +1,2 @@ +Fix an issue where :func:`zip` may create untracked reference cyles. Patch +by Brandt Bucher. From 78d1c5d2bd3ea19c6edb7d0f8485d56e51d8f68c Mon Sep 17 00:00:00 2001 From: Brandt Bucher Date: Thu, 3 Dec 2020 08:30:59 -0800 Subject: [PATCH 04/23] Feedback from code review --- Lib/test/test_builtin.py | 17 ++++++++++------- .../2020-12-02-20-23-31.bpo-42536.Kx3ZOu.rst | 5 +++-- Python/bltinmodule.c | 1 + 3 files changed, 14 insertions(+), 9 deletions(-) diff --git a/Lib/test/test_builtin.py b/Lib/test/test_builtin.py index 2490a038abc711..9d48ed7499b4c4 100644 --- a/Lib/test/test_builtin.py +++ b/Lib/test/test_builtin.py @@ -1757,14 +1757,17 @@ def __next__(self): l8 = self.iter_error(zip(Iter(3), "AB", strict=True), ValueError) self.assertEqual(l8, [(2, "A"), (1, "B")]) + @support.cpython_only def test_zip_result_gc(self): - try: - gc.disable() - z = zip([[]]) - gc.collect() - self.assertTrue(gc.is_tracked(next(z))) - finally: - gc.enable() + # bpo-42536: zip's tuple-reuse speed trick breaks the GC's assumptions + # about what can be un-tracked. Make sure we re-track result tuples + # whenever we reuse them. + z = zip([[]]) + gc.collect() + # That GC collection probably untracked the recycled internal result + # tuple, which is initialized to (None,). Make sure it's re-tracked when + # it's mutated and returned from __next__: + self.assertTrue(gc.is_tracked(next(z))) def test_format(self): # Test the basic machinery of the format() builtin. Don't test diff --git a/Misc/NEWS.d/next/Core and Builtins/2020-12-02-20-23-31.bpo-42536.Kx3ZOu.rst b/Misc/NEWS.d/next/Core and Builtins/2020-12-02-20-23-31.bpo-42536.Kx3ZOu.rst index ede2044f6d04f0..f1ebdbe2efd55c 100644 --- a/Misc/NEWS.d/next/Core and Builtins/2020-12-02-20-23-31.bpo-42536.Kx3ZOu.rst +++ b/Misc/NEWS.d/next/Core and Builtins/2020-12-02-20-23-31.bpo-42536.Kx3ZOu.rst @@ -1,2 +1,3 @@ -Fix an issue where :func:`zip` may create untracked reference cyles. Patch -by Brandt Bucher. +:func:`zip` now ensures that its internal result tuple is always tracked by the +garbage collector. Previously, it could have become untracked by a prior garbage +collection. Patch by Brandt Bucher. diff --git a/Python/bltinmodule.c b/Python/bltinmodule.c index 0691cb417bd442..a5ac34addb0239 100644 --- a/Python/bltinmodule.c +++ b/Python/bltinmodule.c @@ -2636,6 +2636,7 @@ zip_next(zipobject *lz) PyTuple_SET_ITEM(result, i, item); Py_DECREF(olditem); } + // bpo-42536 // The GC may have untracked this result tuple if its elements were all // untracked. Since we're recycling it, make sure it's tracked again: if (!_PyObject_GC_IS_TRACKED(result)) { From 698e337d89c4959cdc359c235c1c3216a98d58f5 Mon Sep 17 00:00:00 2001 From: Brandt Bucher Date: Thu, 3 Dec 2020 17:11:31 -0800 Subject: [PATCH 05/23] Add failing regression test for enumerate --- Lib/test/test_enumerate.py | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/Lib/test/test_enumerate.py b/Lib/test/test_enumerate.py index 5785cb46492ef7..1efc35446d1aa7 100644 --- a/Lib/test/test_enumerate.py +++ b/Lib/test/test_enumerate.py @@ -2,6 +2,7 @@ import operator import sys import pickle +import gc from test import support @@ -134,6 +135,25 @@ def test_tuple_reuse(self): self.assertEqual(len(set(map(id, list(enumerate(self.seq))))), len(self.seq)) self.assertEqual(len(set(map(id, enumerate(self.seq)))), min(1,len(self.seq))) + @support.cpython_only + def test_enumerate_result_gc(self): + # bpo-42536: enumerate's tuple-reuse speed trick breaks the GC's + # assumptions about what can be un-tracked. Make sure we re-track result + # tuples whenever we reuse them. + e = self.enum([[]]) + gc.collect() + # That GC collection probably untracked the recycled internal result + # tuple, which is initialized to (None, None). Make sure it's re-tracked + # when it's mutated and returned from __next__: + self.assertTrue(gc.is_tracked(next(e))) + + # @support.cpython_only + # def test_enumerate_long_result_gc(self): + # # Same as test_enumerate_result_gc, above. + # e = self.enum([[]], sys.maxsize) + # gc.collect() + # self.assertTrue(gc.is_tracked(next(e))) + class MyEnum(enumerate): pass From a41a132958f07e32518b5c7b6878f63145001ee9 Mon Sep 17 00:00:00 2001 From: Brandt Bucher Date: Thu, 3 Dec 2020 17:12:05 -0800 Subject: [PATCH 06/23] Re-track recycled enumerate result tuples --- Objects/enumobject.c | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/Objects/enumobject.c b/Objects/enumobject.c index 8b5e7d3a3c6dd5..b02271420fe6fe 100644 --- a/Objects/enumobject.c +++ b/Objects/enumobject.c @@ -2,6 +2,7 @@ #include "Python.h" #include "pycore_long.h" // _PyLong_GetOne() +#include "pycore_object.h" // _PyObject_GC_TRACK() #include "clinic/enumobject.c.h" @@ -131,6 +132,12 @@ enum_next_long(enumobject *en, PyObject* next_item) PyTuple_SET_ITEM(result, 1, next_item); Py_DECREF(old_index); Py_DECREF(old_item); + // bpo-42536 + // The GC may have untracked this result tuple if its elements were all + // untracked. Since we're recycling it, make sure it's tracked again: + if (!_PyObject_GC_IS_TRACKED(result)) { + _PyObject_GC_TRACK(result); + } return result; } result = PyTuple_New(2); @@ -176,6 +183,12 @@ enum_next(enumobject *en) PyTuple_SET_ITEM(result, 1, next_item); Py_DECREF(old_index); Py_DECREF(old_item); + // bpo-42536 + // The GC may have untracked this result tuple if its elements were all + // untracked. Since we're recycling it, make sure it's tracked again: + if (!_PyObject_GC_IS_TRACKED(result)) { + _PyObject_GC_TRACK(result); + } return result; } result = PyTuple_New(2); From 8b7e96a3f8d7d2b5657e7f90bbbed5c0e6cc85c9 Mon Sep 17 00:00:00 2001 From: Brandt Bucher Date: Thu, 3 Dec 2020 17:24:20 -0800 Subject: [PATCH 07/23] Add failing regression tests for dict.items --- Lib/test/test_dict.py | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/Lib/test/test_dict.py b/Lib/test/test_dict.py index 9ff8b7d501aad6..6a001deb3fd3c2 100644 --- a/Lib/test/test_dict.py +++ b/Lib/test/test_dict.py @@ -1452,6 +1452,25 @@ def items(self): d = CustomReversedDict(pairs) self.assertEqual(pairs[::-1], list(dict(d).items())) + @support.cpython_only + def test_dict_items_result_gc(self): + # bpo-42536: dict.items's tuple-reuse speed trick breaks the GC's + # assumptions about what can be un-tracked. Make sure we re-track result + # tuples whenever we reuse them. + i = iter({None: []}.items()) + gc.collect() + # That GC collection probably untracked the recycled internal result + # tuple, which is initialized to (None, None). Make sure it's re-tracked + # when it's mutated and returned from __next__: + self.assertTrue(gc.is_tracked(next(i))) + + @support.cpython_only + def test_dict_items_result_gc(self): + # Same as test_dict_items_result_gc above, but reversed. + i = reversed({None: []}.items()) + gc.collect() + self.assertTrue(gc.is_tracked(next(i))) + class CAPITest(unittest.TestCase): From 2ac50cffc315a4d98543b0a561ab628f2c3596e9 Mon Sep 17 00:00:00 2001 From: Brandt Bucher Date: Thu, 3 Dec 2020 17:24:42 -0800 Subject: [PATCH 08/23] Re-track recycled dict.items result tuples --- Objects/dictobject.c | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/Objects/dictobject.c b/Objects/dictobject.c index 7a37313df8a6b1..65e9f2e230f463 100644 --- a/Objects/dictobject.c +++ b/Objects/dictobject.c @@ -3989,6 +3989,12 @@ dictiter_iternextitem(dictiterobject *di) Py_INCREF(result); Py_DECREF(oldkey); Py_DECREF(oldvalue); + // bpo-42536 + // The GC may have untracked this result tuple if its elements were all + // untracked. Since we're recycling it, make sure it's tracked again: + if (!_PyObject_GC_IS_TRACKED(result)) { + _PyObject_GC_TRACK(result); + } } else { result = PyTuple_New(2); @@ -4104,6 +4110,13 @@ dictreviter_iternext(dictiterobject *di) Py_INCREF(result); Py_DECREF(oldkey); Py_DECREF(oldvalue); + // bpo-42536 + // The GC may have untracked this result tuple if its elements were + // all untracked. Since we're recycling it, make sure it's tracked + // again: + if (!_PyObject_GC_IS_TRACKED(result)) { + _PyObject_GC_TRACK(result); + } } else { result = PyTuple_New(2); From f0aee473c97891e02708f7ef2830cfb2ade48ec1 Mon Sep 17 00:00:00 2001 From: Brandt Bucher Date: Thu, 3 Dec 2020 17:52:01 -0800 Subject: [PATCH 09/23] Add failing regression test for OrderedDict.items --- Lib/test/test_ordered_dict.py | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/Lib/test/test_ordered_dict.py b/Lib/test/test_ordered_dict.py index 31759f20d28349..0b93dfada47b6e 100644 --- a/Lib/test/test_ordered_dict.py +++ b/Lib/test/test_ordered_dict.py @@ -700,6 +700,17 @@ def test_merge_operator(self): with self.assertRaises(ValueError): a |= "BAD" + @support.cpython_only + def test_ordered_dict_items_result_gc(self): + # bpo-42536: OrderedDict.item's tuple-reuse speed trick breaks the GC's + # assumptions about what can be un-tracked. Make sure we re-track result + # tuples whenever we reuse them. + i = iter(self.OrderedDict({None: []}).items()) + gc.collect() + # That GC collection probably untracked the recycled internal result + # tuple, which is initialized to (None, None). Make sure it's re-tracked + # when it's mutated and returned from __next__: + self.assertTrue(gc.is_tracked(next(i))) class PurePythonOrderedDictTests(OrderedDictTests, unittest.TestCase): From 363256ba447e6d5f405a1be123822e8a360b2793 Mon Sep 17 00:00:00 2001 From: Brandt Bucher Date: Thu, 3 Dec 2020 17:52:25 -0800 Subject: [PATCH 10/23] Re-track recycled OrderedDict.items result tuples --- Objects/odictobject.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/Objects/odictobject.c b/Objects/odictobject.c index 4eb15f999bd1e4..6709a77ccca172 100644 --- a/Objects/odictobject.c +++ b/Objects/odictobject.c @@ -1814,6 +1814,12 @@ odictiter_iternext(odictiterobject *di) Py_INCREF(result); Py_DECREF(PyTuple_GET_ITEM(result, 0)); /* borrowed */ Py_DECREF(PyTuple_GET_ITEM(result, 1)); /* borrowed */ + // bpo-42536 + // The GC may have untracked this result tuple if its elements were all + // untracked. Since we're recycling it, make sure it's tracked again: + if (!_PyObject_GC_IS_TRACKED(result)) { + _PyObject_GC_TRACK(result); + } } else { result = PyTuple_New(2); From 63db279b42e94b8675a0658da6e0845efd04892f Mon Sep 17 00:00:00 2001 From: Brandt Bucher Date: Thu, 3 Dec 2020 18:36:34 -0800 Subject: [PATCH 11/23] Add failing regression tests for itertools --- Lib/test/test_itertools.py | 66 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 66 insertions(+) diff --git a/Lib/test/test_itertools.py b/Lib/test/test_itertools.py index df2997e87d494c..6f8c83d8ad9587 100644 --- a/Lib/test/test_itertools.py +++ b/Lib/test/test_itertools.py @@ -12,6 +12,8 @@ import sys import struct import threading +import gc + maxsize = support.MAX_Py_ssize_t minsize = -maxsize-1 @@ -294,6 +296,19 @@ def test_combinations_tuple_reuse(self): self.assertEqual(len(set(map(id, combinations('abcde', 3)))), 1) self.assertNotEqual(len(set(map(id, list(combinations('abcde', 3))))), 1) + @support.cpython_only + def test_combinations_result_gc(self): + # bpo-42536: combinations's tuple-reuse speed trick breaks the GC's + # assumptions about what can be un-tracked. Make sure we re-track result + # tuples whenever we reuse them. + c = combinations([None, []], 1) + next(c) + gc.collect() + # That GC collection probably untracked the recycled internal result + # tuple, which has the value (None,). Make sure it's re-tracked when + # it's mutated and returned from __next__: + self.assertTrue(gc.is_tracked(next(c))) + def test_combinations_with_replacement(self): cwr = combinations_with_replacement self.assertRaises(TypeError, cwr, 'abc') # missing r argument @@ -382,6 +397,19 @@ def test_combinations_with_replacement_tuple_reuse(self): self.assertEqual(len(set(map(id, cwr('abcde', 3)))), 1) self.assertNotEqual(len(set(map(id, list(cwr('abcde', 3))))), 1) + @support.cpython_only + def test_combinations_with_replacement_result_gc(self): + # bpo-42536: combinations_with_replacement's tuple-reuse speed trick + # breaks the GC's assumptions about what can be un-tracked. Make sure we + # re-track result tuples whenever we reuse them. + c = combinations_with_replacement([None, []], 1) + next(c) + gc.collect() + # That GC collection probably untracked the recycled internal result + # tuple, which has the value (None,). Make sure it's re-tracked when + # it's mutated and returned from __next__: + self.assertTrue(gc.is_tracked(next(c))) + def test_permutations(self): self.assertRaises(TypeError, permutations) # too few arguments self.assertRaises(TypeError, permutations, 'abc', 2, 1) # too many arguments @@ -455,6 +483,19 @@ def test_permutations_tuple_reuse(self): self.assertEqual(len(set(map(id, permutations('abcde', 3)))), 1) self.assertNotEqual(len(set(map(id, list(permutations('abcde', 3))))), 1) + @support.cpython_only + def test_permutations_result_gc(self): + # bpo-42536: combinations's tuple-reuse speed trick breaks the GC's + # assumptions about what can be un-tracked. Make sure we re-track result + # tuples whenever we reuse them. + p = permutations([None, []], 1) + next(p) + gc.collect() + # That GC collection probably untracked the recycled internal result + # tuple, which has the value (None,). Make sure it's re-tracked when + # it's mutated and returned from __next__: + self.assertTrue(gc.is_tracked(next(p))) + def test_combinatorics(self): # Test relationships between product(), permutations(), # combinations() and combinations_with_replacement(). @@ -965,6 +1006,18 @@ def test_zip_longest_tuple_reuse(self): ids = list(map(id, list(zip_longest('abc', 'def')))) self.assertEqual(len(dict.fromkeys(ids)), len(ids)) + @support.cpython_only + def test_zip_longest_result_gc(self): + # bpo-42536: zip_longest's tuple-reuse speed trick breaks the GC's + # assumptions about what can be un-tracked. Make sure we re-track result + # tuples whenever we reuse them. + z = zip_longest([[]]) + gc.collect() + # That GC collection probably untracked the recycled internal result + # tuple, which has the value (None,). Make sure it's re-tracked when + # it's mutated and returned from __next__: + self.assertTrue(gc.is_tracked(next(z))) + def test_zip_longest_pickling(self): for proto in range(pickle.HIGHEST_PROTOCOL + 1): self.pickletest(proto, zip_longest("abc", "def")) @@ -1111,6 +1164,19 @@ def test_product_tuple_reuse(self): self.assertEqual(len(set(map(id, product('abc', 'def')))), 1) self.assertNotEqual(len(set(map(id, list(product('abc', 'def'))))), 1) + @support.cpython_only + def test_product_result_gc(self): + # bpo-42536: produce's tuple-reuse speed trick breaks the GC's + # assumptions about what can be un-tracked. Make sure we re-track result + # tuples whenever we reuse them. + p = product([None, []]) + next(p) + gc.collect() + # That GC collection probably untracked the recycled internal result + # tuple, which has the value (None,). Make sure it's re-tracked when + # it's mutated and returned from __next__: + self.assertTrue(gc.is_tracked(next(p))) + def test_product_pickling(self): # check copy, deepcopy, pickle for args, result in [ From 027a51f1f037bcfd9f2314267dc85cfc5ad81ebd Mon Sep 17 00:00:00 2001 From: Brandt Bucher Date: Thu, 3 Dec 2020 18:37:50 -0800 Subject: [PATCH 12/23] Re-track recycled itertools result tuples --- Modules/itertoolsmodule.c | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/Modules/itertoolsmodule.c b/Modules/itertoolsmodule.c index 7144856c352f81..169dd0eabfdf72 100644 --- a/Modules/itertoolsmodule.c +++ b/Modules/itertoolsmodule.c @@ -3,6 +3,7 @@ #define PY_SSIZE_T_CLEAN #include "Python.h" #include "pycore_long.h" // _PyLong_GetZero() +#include "pycore_object.h" // _PyObject_GC_TRACK() #include "pycore_tuple.h" // _PyTuple_ITEMS() #include // offsetof() @@ -2378,6 +2379,12 @@ product_next(productobject *lz) lz->result = result; Py_DECREF(old_result); } + // bpo-42536 + // The GC may have untracked this result tuple if its elements were all + // untracked. Since we're recycling it, make sure it's tracked again: + else if (!_PyObject_GC_IS_TRACKED(result)) { + _PyObject_GC_TRACK(result); + } /* Now, we've got the only copy so we can update it in-place */ assert (npools==0 || Py_REFCNT(result) == 1); @@ -2701,6 +2708,12 @@ combinations_next(combinationsobject *co) co->result = result; Py_DECREF(old_result); } + // bpo-42536 + // The GC may have untracked this result tuple if its elements were all + // untracked. Since we're recycling it, make sure it's tracked again: + else if (!_PyObject_GC_IS_TRACKED(result)) { + _PyObject_GC_TRACK(result); + } /* Now, we've got the only copy so we can update it in-place * CPython's empty tuple is a singleton and cached in * PyTuple's freelist. @@ -3035,6 +3048,12 @@ cwr_next(cwrobject *co) co->result = result; Py_DECREF(old_result); } + // bpo-42536 + // The GC may have untracked this result tuple if its elements were all + // untracked. Since we're recycling it, make sure it's tracked again: + else if (!_PyObject_GC_IS_TRACKED(result)) { + _PyObject_GC_TRACK(result); + } /* Now, we've got the only copy so we can update it in-place CPython's empty tuple is a singleton and cached in PyTuple's freelist. */ assert(r == 0 || Py_REFCNT(result) == 1); @@ -3379,6 +3398,12 @@ permutations_next(permutationsobject *po) po->result = result; Py_DECREF(old_result); } + // bpo-42536 + // The GC may have untracked this result tuple if its elements were all + // untracked. Since we're recycling it, make sure it's tracked again: + else if (!_PyObject_GC_IS_TRACKED(result)) { + _PyObject_GC_TRACK(result); + } /* Now, we've got the only copy so we can update it in-place */ assert(r == 0 || Py_REFCNT(result) == 1); @@ -4649,6 +4674,12 @@ zip_longest_next(ziplongestobject *lz) PyTuple_SET_ITEM(result, i, item); Py_DECREF(olditem); } + // bpo-42536 + // The GC may have untracked this result tuple if its elements were all + // untracked. Since we're recycling it, make sure it's tracked again: + if (!_PyObject_GC_IS_TRACKED(result)) { + _PyObject_GC_TRACK(result); + } } else { result = PyTuple_New(tuplesize); if (result == NULL) From 4053bc6c763eb20ff2422fdec9c30a5224a62207 Mon Sep 17 00:00:00 2001 From: Brandt Bucher Date: Thu, 3 Dec 2020 18:43:17 -0800 Subject: [PATCH 13/23] Reword comments --- Lib/test/test_itertools.py | 29 +++++------------------------ Modules/itertoolsmodule.c | 25 ++++++++++--------------- Objects/dictobject.c | 11 ++++------- Objects/enumobject.c | 10 ++++------ Objects/odictobject.c | 5 ++--- Python/bltinmodule.c | 5 ++--- 6 files changed, 27 insertions(+), 58 deletions(-) diff --git a/Lib/test/test_itertools.py b/Lib/test/test_itertools.py index 6f8c83d8ad9587..0df0debb042c9f 100644 --- a/Lib/test/test_itertools.py +++ b/Lib/test/test_itertools.py @@ -399,15 +399,11 @@ def test_combinations_with_replacement_tuple_reuse(self): @support.cpython_only def test_combinations_with_replacement_result_gc(self): - # bpo-42536: combinations_with_replacement's tuple-reuse speed trick - # breaks the GC's assumptions about what can be un-tracked. Make sure we - # re-track result tuples whenever we reuse them. + # Same as test_combinations_result_gc above, but for + # combinations_with_replacement. c = combinations_with_replacement([None, []], 1) next(c) gc.collect() - # That GC collection probably untracked the recycled internal result - # tuple, which has the value (None,). Make sure it's re-tracked when - # it's mutated and returned from __next__: self.assertTrue(gc.is_tracked(next(c))) def test_permutations(self): @@ -485,15 +481,10 @@ def test_permutations_tuple_reuse(self): @support.cpython_only def test_permutations_result_gc(self): - # bpo-42536: combinations's tuple-reuse speed trick breaks the GC's - # assumptions about what can be un-tracked. Make sure we re-track result - # tuples whenever we reuse them. + # Same as test_combinations_result_gc above, but for permutations. p = permutations([None, []], 1) next(p) gc.collect() - # That GC collection probably untracked the recycled internal result - # tuple, which has the value (None,). Make sure it's re-tracked when - # it's mutated and returned from __next__: self.assertTrue(gc.is_tracked(next(p))) def test_combinatorics(self): @@ -1008,14 +999,9 @@ def test_zip_longest_tuple_reuse(self): @support.cpython_only def test_zip_longest_result_gc(self): - # bpo-42536: zip_longest's tuple-reuse speed trick breaks the GC's - # assumptions about what can be un-tracked. Make sure we re-track result - # tuples whenever we reuse them. + # Same as test_combinations_result_gc above, but for zip_longest. z = zip_longest([[]]) gc.collect() - # That GC collection probably untracked the recycled internal result - # tuple, which has the value (None,). Make sure it's re-tracked when - # it's mutated and returned from __next__: self.assertTrue(gc.is_tracked(next(z))) def test_zip_longest_pickling(self): @@ -1166,15 +1152,10 @@ def test_product_tuple_reuse(self): @support.cpython_only def test_product_result_gc(self): - # bpo-42536: produce's tuple-reuse speed trick breaks the GC's - # assumptions about what can be un-tracked. Make sure we re-track result - # tuples whenever we reuse them. + # Same as test_combinations_result_gc above, but for product. p = product([None, []]) next(p) gc.collect() - # That GC collection probably untracked the recycled internal result - # tuple, which has the value (None,). Make sure it's re-tracked when - # it's mutated and returned from __next__: self.assertTrue(gc.is_tracked(next(p))) def test_product_pickling(self): diff --git a/Modules/itertoolsmodule.c b/Modules/itertoolsmodule.c index 169dd0eabfdf72..2315816da5617e 100644 --- a/Modules/itertoolsmodule.c +++ b/Modules/itertoolsmodule.c @@ -2379,9 +2379,8 @@ product_next(productobject *lz) lz->result = result; Py_DECREF(old_result); } - // bpo-42536 - // The GC may have untracked this result tuple if its elements were all - // untracked. Since we're recycling it, make sure it's tracked again: + // bpo-42536: the GC may have untracked this result tuple. Since we're + // recycling it, make sure it's tracked again: else if (!_PyObject_GC_IS_TRACKED(result)) { _PyObject_GC_TRACK(result); } @@ -2708,9 +2707,8 @@ combinations_next(combinationsobject *co) co->result = result; Py_DECREF(old_result); } - // bpo-42536 - // The GC may have untracked this result tuple if its elements were all - // untracked. Since we're recycling it, make sure it's tracked again: + // bpo-42536: the GC may have untracked this result tuple. Since we're + // recycling it, make sure it's tracked again: else if (!_PyObject_GC_IS_TRACKED(result)) { _PyObject_GC_TRACK(result); } @@ -3048,9 +3046,8 @@ cwr_next(cwrobject *co) co->result = result; Py_DECREF(old_result); } - // bpo-42536 - // The GC may have untracked this result tuple if its elements were all - // untracked. Since we're recycling it, make sure it's tracked again: + // bpo-42536: the GC may have untracked this result tuple. Since we're + // recycling it, make sure it's tracked again: else if (!_PyObject_GC_IS_TRACKED(result)) { _PyObject_GC_TRACK(result); } @@ -3398,9 +3395,8 @@ permutations_next(permutationsobject *po) po->result = result; Py_DECREF(old_result); } - // bpo-42536 - // The GC may have untracked this result tuple if its elements were all - // untracked. Since we're recycling it, make sure it's tracked again: + // bpo-42536: the GC may have untracked this result tuple. Since we're + // recycling it, make sure it's tracked again: else if (!_PyObject_GC_IS_TRACKED(result)) { _PyObject_GC_TRACK(result); } @@ -4674,9 +4670,8 @@ zip_longest_next(ziplongestobject *lz) PyTuple_SET_ITEM(result, i, item); Py_DECREF(olditem); } - // bpo-42536 - // The GC may have untracked this result tuple if its elements were all - // untracked. Since we're recycling it, make sure it's tracked again: + // bpo-42536: the GC may have untracked this result tuple. Since we're + // recycling it, make sure it's tracked again: if (!_PyObject_GC_IS_TRACKED(result)) { _PyObject_GC_TRACK(result); } diff --git a/Objects/dictobject.c b/Objects/dictobject.c index 65e9f2e230f463..16d0cd39382089 100644 --- a/Objects/dictobject.c +++ b/Objects/dictobject.c @@ -3989,9 +3989,8 @@ dictiter_iternextitem(dictiterobject *di) Py_INCREF(result); Py_DECREF(oldkey); Py_DECREF(oldvalue); - // bpo-42536 - // The GC may have untracked this result tuple if its elements were all - // untracked. Since we're recycling it, make sure it's tracked again: + // bpo-42536: the GC may have untracked this result tuple. Since we're + // recycling it, make sure it's tracked again: if (!_PyObject_GC_IS_TRACKED(result)) { _PyObject_GC_TRACK(result); } @@ -4110,10 +4109,8 @@ dictreviter_iternext(dictiterobject *di) Py_INCREF(result); Py_DECREF(oldkey); Py_DECREF(oldvalue); - // bpo-42536 - // The GC may have untracked this result tuple if its elements were - // all untracked. Since we're recycling it, make sure it's tracked - // again: + // bpo-42536: the GC may have untracked this result tuple. Since + // we're recycling it, make sure it's tracked again: if (!_PyObject_GC_IS_TRACKED(result)) { _PyObject_GC_TRACK(result); } diff --git a/Objects/enumobject.c b/Objects/enumobject.c index b02271420fe6fe..d414f9d076e7d7 100644 --- a/Objects/enumobject.c +++ b/Objects/enumobject.c @@ -132,9 +132,8 @@ enum_next_long(enumobject *en, PyObject* next_item) PyTuple_SET_ITEM(result, 1, next_item); Py_DECREF(old_index); Py_DECREF(old_item); - // bpo-42536 - // The GC may have untracked this result tuple if its elements were all - // untracked. Since we're recycling it, make sure it's tracked again: + // bpo-42536: the GC may have untracked this result tuple. Since we're + // recycling it, make sure it's tracked again: if (!_PyObject_GC_IS_TRACKED(result)) { _PyObject_GC_TRACK(result); } @@ -183,9 +182,8 @@ enum_next(enumobject *en) PyTuple_SET_ITEM(result, 1, next_item); Py_DECREF(old_index); Py_DECREF(old_item); - // bpo-42536 - // The GC may have untracked this result tuple if its elements were all - // untracked. Since we're recycling it, make sure it's tracked again: + // bpo-42536: the GC may have untracked this result tuple. Since we're + // recycling it, make sure it's tracked again: if (!_PyObject_GC_IS_TRACKED(result)) { _PyObject_GC_TRACK(result); } diff --git a/Objects/odictobject.c b/Objects/odictobject.c index 6709a77ccca172..2e72e47197203f 100644 --- a/Objects/odictobject.c +++ b/Objects/odictobject.c @@ -1814,9 +1814,8 @@ odictiter_iternext(odictiterobject *di) Py_INCREF(result); Py_DECREF(PyTuple_GET_ITEM(result, 0)); /* borrowed */ Py_DECREF(PyTuple_GET_ITEM(result, 1)); /* borrowed */ - // bpo-42536 - // The GC may have untracked this result tuple if its elements were all - // untracked. Since we're recycling it, make sure it's tracked again: + // bpo-42536: the GC may have untracked this result tuple. Since we're + // recycling it, make sure it's tracked again: if (!_PyObject_GC_IS_TRACKED(result)) { _PyObject_GC_TRACK(result); } diff --git a/Python/bltinmodule.c b/Python/bltinmodule.c index a5ac34addb0239..d2d1e72ffcb9c0 100644 --- a/Python/bltinmodule.c +++ b/Python/bltinmodule.c @@ -2636,9 +2636,8 @@ zip_next(zipobject *lz) PyTuple_SET_ITEM(result, i, item); Py_DECREF(olditem); } - // bpo-42536 - // The GC may have untracked this result tuple if its elements were all - // untracked. Since we're recycling it, make sure it's tracked again: + // bpo-42536: the GC may have untracked this result tuple. Since we're + // recycling it, make sure it's tracked again: if (!_PyObject_GC_IS_TRACKED(result)) { _PyObject_GC_TRACK(result); } From 83cdf039f2e17f17290e9d9e5c0ba42622b8b5a5 Mon Sep 17 00:00:00 2001 From: Brandt Bucher Date: Thu, 3 Dec 2020 19:13:07 -0800 Subject: [PATCH 14/23] Reword comments --- Lib/test/test_builtin.py | 2 +- Lib/test/test_dict.py | 2 +- Lib/test/test_enumerate.py | 2 +- Lib/test/test_itertools.py | 2 +- Lib/test/test_ordered_dict.py | 2 +- 5 files changed, 5 insertions(+), 5 deletions(-) diff --git a/Lib/test/test_builtin.py b/Lib/test/test_builtin.py index 9d48ed7499b4c4..fe903929e57618 100644 --- a/Lib/test/test_builtin.py +++ b/Lib/test/test_builtin.py @@ -1760,7 +1760,7 @@ def __next__(self): @support.cpython_only def test_zip_result_gc(self): # bpo-42536: zip's tuple-reuse speed trick breaks the GC's assumptions - # about what can be un-tracked. Make sure we re-track result tuples + # about what can be untracked. Make sure we re-track result tuples # whenever we reuse them. z = zip([[]]) gc.collect() diff --git a/Lib/test/test_dict.py b/Lib/test/test_dict.py index 6a001deb3fd3c2..5e940a18b20de1 100644 --- a/Lib/test/test_dict.py +++ b/Lib/test/test_dict.py @@ -1455,7 +1455,7 @@ def items(self): @support.cpython_only def test_dict_items_result_gc(self): # bpo-42536: dict.items's tuple-reuse speed trick breaks the GC's - # assumptions about what can be un-tracked. Make sure we re-track result + # assumptions about what can be untracked. Make sure we re-track result # tuples whenever we reuse them. i = iter({None: []}.items()) gc.collect() diff --git a/Lib/test/test_enumerate.py b/Lib/test/test_enumerate.py index 1efc35446d1aa7..3390c37edfa3f3 100644 --- a/Lib/test/test_enumerate.py +++ b/Lib/test/test_enumerate.py @@ -138,7 +138,7 @@ def test_tuple_reuse(self): @support.cpython_only def test_enumerate_result_gc(self): # bpo-42536: enumerate's tuple-reuse speed trick breaks the GC's - # assumptions about what can be un-tracked. Make sure we re-track result + # assumptions about what can be untracked. Make sure we re-track result # tuples whenever we reuse them. e = self.enum([[]]) gc.collect() diff --git a/Lib/test/test_itertools.py b/Lib/test/test_itertools.py index 0df0debb042c9f..6a44582ea1cc01 100644 --- a/Lib/test/test_itertools.py +++ b/Lib/test/test_itertools.py @@ -299,7 +299,7 @@ def test_combinations_tuple_reuse(self): @support.cpython_only def test_combinations_result_gc(self): # bpo-42536: combinations's tuple-reuse speed trick breaks the GC's - # assumptions about what can be un-tracked. Make sure we re-track result + # assumptions about what can be untracked. Make sure we re-track result # tuples whenever we reuse them. c = combinations([None, []], 1) next(c) diff --git a/Lib/test/test_ordered_dict.py b/Lib/test/test_ordered_dict.py index 0b93dfada47b6e..f2ec80352c4b37 100644 --- a/Lib/test/test_ordered_dict.py +++ b/Lib/test/test_ordered_dict.py @@ -703,7 +703,7 @@ def test_merge_operator(self): @support.cpython_only def test_ordered_dict_items_result_gc(self): # bpo-42536: OrderedDict.item's tuple-reuse speed trick breaks the GC's - # assumptions about what can be un-tracked. Make sure we re-track result + # assumptions about what can be untracked. Make sure we re-track result # tuples whenever we reuse them. i = iter(self.OrderedDict({None: []}).items()) gc.collect() From 3583ba44b9a575653c4fc613343c1f32f8f9ac04 Mon Sep 17 00:00:00 2001 From: Brandt Bucher Date: Thu, 3 Dec 2020 19:14:17 -0800 Subject: [PATCH 15/23] Re-track recycled reduce arg tuples --- Modules/_functoolsmodule.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/Modules/_functoolsmodule.c b/Modules/_functoolsmodule.c index ff03c334766b89..cb85ce93ab207b 100644 --- a/Modules/_functoolsmodule.c +++ b/Modules/_functoolsmodule.c @@ -1,5 +1,6 @@ #include "Python.h" #include "pycore_long.h" // _PyLong_GetZero() +#include "pycore_object.h" // _PyObject_GC_TRACK #include "pycore_pystate.h" // _PyThreadState_GET() #include "pycore_tuple.h" // _PyTuple_ITEMS() #include "structmember.h" // PyMemberDef @@ -673,6 +674,11 @@ functools_reduce(PyObject *self, PyObject *args) if ((result = PyObject_Call(func, args, NULL)) == NULL) { goto Fail; } + // bpo-42536: the GC may have untracked this args tuple. Since we're + // recycling it, make sure it's tracked again: + if (!_PyObject_GC_IS_TRACKED(args)) { + _PyObject_GC_TRACK(args); + } } } From 1ac7590f6c0a36fbc59a700c0a311daf233008b6 Mon Sep 17 00:00:00 2001 From: Brandt Bucher Date: Thu, 3 Dec 2020 19:14:49 -0800 Subject: [PATCH 16/23] Reword comments --- Modules/_functoolsmodule.c | 2 +- Modules/itertoolsmodule.c | 10 +++++----- Objects/dictobject.c | 4 ++-- Objects/enumobject.c | 4 ++-- Objects/odictobject.c | 2 +- Python/bltinmodule.c | 2 +- 6 files changed, 12 insertions(+), 12 deletions(-) diff --git a/Modules/_functoolsmodule.c b/Modules/_functoolsmodule.c index cb85ce93ab207b..621b721d011df7 100644 --- a/Modules/_functoolsmodule.c +++ b/Modules/_functoolsmodule.c @@ -674,7 +674,7 @@ functools_reduce(PyObject *self, PyObject *args) if ((result = PyObject_Call(func, args, NULL)) == NULL) { goto Fail; } - // bpo-42536: the GC may have untracked this args tuple. Since we're + // bpo-42536: The GC may have untracked this args tuple. Since we're // recycling it, make sure it's tracked again: if (!_PyObject_GC_IS_TRACKED(args)) { _PyObject_GC_TRACK(args); diff --git a/Modules/itertoolsmodule.c b/Modules/itertoolsmodule.c index 2315816da5617e..293735a886428b 100644 --- a/Modules/itertoolsmodule.c +++ b/Modules/itertoolsmodule.c @@ -2379,7 +2379,7 @@ product_next(productobject *lz) lz->result = result; Py_DECREF(old_result); } - // bpo-42536: the GC may have untracked this result tuple. Since we're + // bpo-42536: The GC may have untracked this result tuple. Since we're // recycling it, make sure it's tracked again: else if (!_PyObject_GC_IS_TRACKED(result)) { _PyObject_GC_TRACK(result); @@ -2707,7 +2707,7 @@ combinations_next(combinationsobject *co) co->result = result; Py_DECREF(old_result); } - // bpo-42536: the GC may have untracked this result tuple. Since we're + // bpo-42536: The GC may have untracked this result tuple. Since we're // recycling it, make sure it's tracked again: else if (!_PyObject_GC_IS_TRACKED(result)) { _PyObject_GC_TRACK(result); @@ -3046,7 +3046,7 @@ cwr_next(cwrobject *co) co->result = result; Py_DECREF(old_result); } - // bpo-42536: the GC may have untracked this result tuple. Since we're + // bpo-42536: The GC may have untracked this result tuple. Since we're // recycling it, make sure it's tracked again: else if (!_PyObject_GC_IS_TRACKED(result)) { _PyObject_GC_TRACK(result); @@ -3395,7 +3395,7 @@ permutations_next(permutationsobject *po) po->result = result; Py_DECREF(old_result); } - // bpo-42536: the GC may have untracked this result tuple. Since we're + // bpo-42536: The GC may have untracked this result tuple. Since we're // recycling it, make sure it's tracked again: else if (!_PyObject_GC_IS_TRACKED(result)) { _PyObject_GC_TRACK(result); @@ -4670,7 +4670,7 @@ zip_longest_next(ziplongestobject *lz) PyTuple_SET_ITEM(result, i, item); Py_DECREF(olditem); } - // bpo-42536: the GC may have untracked this result tuple. Since we're + // bpo-42536: The GC may have untracked this result tuple. Since we're // recycling it, make sure it's tracked again: if (!_PyObject_GC_IS_TRACKED(result)) { _PyObject_GC_TRACK(result); diff --git a/Objects/dictobject.c b/Objects/dictobject.c index 16d0cd39382089..35e881fe272308 100644 --- a/Objects/dictobject.c +++ b/Objects/dictobject.c @@ -3989,7 +3989,7 @@ dictiter_iternextitem(dictiterobject *di) Py_INCREF(result); Py_DECREF(oldkey); Py_DECREF(oldvalue); - // bpo-42536: the GC may have untracked this result tuple. Since we're + // bpo-42536: The GC may have untracked this result tuple. Since we're // recycling it, make sure it's tracked again: if (!_PyObject_GC_IS_TRACKED(result)) { _PyObject_GC_TRACK(result); @@ -4109,7 +4109,7 @@ dictreviter_iternext(dictiterobject *di) Py_INCREF(result); Py_DECREF(oldkey); Py_DECREF(oldvalue); - // bpo-42536: the GC may have untracked this result tuple. Since + // bpo-42536: The GC may have untracked this result tuple. Since // we're recycling it, make sure it's tracked again: if (!_PyObject_GC_IS_TRACKED(result)) { _PyObject_GC_TRACK(result); diff --git a/Objects/enumobject.c b/Objects/enumobject.c index d414f9d076e7d7..98ece3f13fc6fb 100644 --- a/Objects/enumobject.c +++ b/Objects/enumobject.c @@ -132,7 +132,7 @@ enum_next_long(enumobject *en, PyObject* next_item) PyTuple_SET_ITEM(result, 1, next_item); Py_DECREF(old_index); Py_DECREF(old_item); - // bpo-42536: the GC may have untracked this result tuple. Since we're + // bpo-42536: The GC may have untracked this result tuple. Since we're // recycling it, make sure it's tracked again: if (!_PyObject_GC_IS_TRACKED(result)) { _PyObject_GC_TRACK(result); @@ -182,7 +182,7 @@ enum_next(enumobject *en) PyTuple_SET_ITEM(result, 1, next_item); Py_DECREF(old_index); Py_DECREF(old_item); - // bpo-42536: the GC may have untracked this result tuple. Since we're + // bpo-42536: The GC may have untracked this result tuple. Since we're // recycling it, make sure it's tracked again: if (!_PyObject_GC_IS_TRACKED(result)) { _PyObject_GC_TRACK(result); diff --git a/Objects/odictobject.c b/Objects/odictobject.c index 2e72e47197203f..6c7f1175cd652c 100644 --- a/Objects/odictobject.c +++ b/Objects/odictobject.c @@ -1814,7 +1814,7 @@ odictiter_iternext(odictiterobject *di) Py_INCREF(result); Py_DECREF(PyTuple_GET_ITEM(result, 0)); /* borrowed */ Py_DECREF(PyTuple_GET_ITEM(result, 1)); /* borrowed */ - // bpo-42536: the GC may have untracked this result tuple. Since we're + // bpo-42536: The GC may have untracked this result tuple. Since we're // recycling it, make sure it's tracked again: if (!_PyObject_GC_IS_TRACKED(result)) { _PyObject_GC_TRACK(result); diff --git a/Python/bltinmodule.c b/Python/bltinmodule.c index d2d1e72ffcb9c0..352fb83d55e053 100644 --- a/Python/bltinmodule.c +++ b/Python/bltinmodule.c @@ -2636,7 +2636,7 @@ zip_next(zipobject *lz) PyTuple_SET_ITEM(result, i, item); Py_DECREF(olditem); } - // bpo-42536: the GC may have untracked this result tuple. Since we're + // bpo-42536: The GC may have untracked this result tuple. Since we're // recycling it, make sure it's tracked again: if (!_PyObject_GC_IS_TRACKED(result)) { _PyObject_GC_TRACK(result); From 6e12b8ba50fa035f038d0e3e3975c2209c5eb7ba Mon Sep 17 00:00:00 2001 From: Brandt Bucher Date: Thu, 3 Dec 2020 20:36:08 -0800 Subject: [PATCH 17/23] Remove comment --- Lib/test/test_enumerate.py | 7 ------- 1 file changed, 7 deletions(-) diff --git a/Lib/test/test_enumerate.py b/Lib/test/test_enumerate.py index 3390c37edfa3f3..17a26fedb29519 100644 --- a/Lib/test/test_enumerate.py +++ b/Lib/test/test_enumerate.py @@ -147,13 +147,6 @@ def test_enumerate_result_gc(self): # when it's mutated and returned from __next__: self.assertTrue(gc.is_tracked(next(e))) - # @support.cpython_only - # def test_enumerate_long_result_gc(self): - # # Same as test_enumerate_result_gc, above. - # e = self.enum([[]], sys.maxsize) - # gc.collect() - # self.assertTrue(gc.is_tracked(next(e))) - class MyEnum(enumerate): pass From 80f59e43de9144dbfad7c7df3fbb9b1cc50337d0 Mon Sep 17 00:00:00 2001 From: Brandt Bucher Date: Thu, 3 Dec 2020 20:39:14 -0800 Subject: [PATCH 18/23] Update NEWS entry. --- .../2020-12-02-20-23-31.bpo-42536.Kx3ZOu.rst | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/Misc/NEWS.d/next/Core and Builtins/2020-12-02-20-23-31.bpo-42536.Kx3ZOu.rst b/Misc/NEWS.d/next/Core and Builtins/2020-12-02-20-23-31.bpo-42536.Kx3ZOu.rst index f1ebdbe2efd55c..c6efd6f85d7d78 100644 --- a/Misc/NEWS.d/next/Core and Builtins/2020-12-02-20-23-31.bpo-42536.Kx3ZOu.rst +++ b/Misc/NEWS.d/next/Core and Builtins/2020-12-02-20-23-31.bpo-42536.Kx3ZOu.rst @@ -1,3 +1,4 @@ -:func:`zip` now ensures that its internal result tuple is always tracked by the -garbage collector. Previously, it could have become untracked by a prior garbage -collection. Patch by Brandt Bucher. +Several built-in and standard library types now ensure that their internal +result tuples are always tracked by the garbage collector. Previously, they +could have become untracked by a prior garbage collection. Patch by Brandt +Bucher. From 9005df466dc4c8bbd2ecb724088e30f6d9689125 Mon Sep 17 00:00:00 2001 From: Brandt Bucher Date: Thu, 3 Dec 2020 21:07:34 -0800 Subject: [PATCH 19/23] Fix typo --- Lib/test/test_ordered_dict.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Lib/test/test_ordered_dict.py b/Lib/test/test_ordered_dict.py index f2ec80352c4b37..63f47279c1b57e 100644 --- a/Lib/test/test_ordered_dict.py +++ b/Lib/test/test_ordered_dict.py @@ -702,7 +702,7 @@ def test_merge_operator(self): @support.cpython_only def test_ordered_dict_items_result_gc(self): - # bpo-42536: OrderedDict.item's tuple-reuse speed trick breaks the GC's + # bpo-42536: OrderedDict.items's tuple-reuse speed trick breaks the GC's # assumptions about what can be untracked. Make sure we re-track result # tuples whenever we reuse them. i = iter(self.OrderedDict({None: []}).items()) From 6ec236882a61a56b84ea41ea8c2b24768b6ea8ee Mon Sep 17 00:00:00 2001 From: Brandt Bucher Date: Fri, 4 Dec 2020 10:20:20 -0800 Subject: [PATCH 20/23] Replace single-character names --- Lib/test/test_builtin.py | 4 ++-- Lib/test/test_dict.py | 8 ++++---- Lib/test/test_enumerate.py | 4 ++-- Lib/test/test_itertools.py | 28 ++++++++++++++-------------- Lib/test/test_ordered_dict.py | 4 ++-- 5 files changed, 24 insertions(+), 24 deletions(-) diff --git a/Lib/test/test_builtin.py b/Lib/test/test_builtin.py index fe903929e57618..8c9573731ae2ed 100644 --- a/Lib/test/test_builtin.py +++ b/Lib/test/test_builtin.py @@ -1762,12 +1762,12 @@ def test_zip_result_gc(self): # bpo-42536: zip's tuple-reuse speed trick breaks the GC's assumptions # about what can be untracked. Make sure we re-track result tuples # whenever we reuse them. - z = zip([[]]) + it = zip([[]]) gc.collect() # That GC collection probably untracked the recycled internal result # tuple, which is initialized to (None,). Make sure it's re-tracked when # it's mutated and returned from __next__: - self.assertTrue(gc.is_tracked(next(z))) + self.assertTrue(gc.is_tracked(next(it))) def test_format(self): # Test the basic machinery of the format() builtin. Don't test diff --git a/Lib/test/test_dict.py b/Lib/test/test_dict.py index 5e940a18b20de1..4b31cdc79415ff 100644 --- a/Lib/test/test_dict.py +++ b/Lib/test/test_dict.py @@ -1457,19 +1457,19 @@ def test_dict_items_result_gc(self): # bpo-42536: dict.items's tuple-reuse speed trick breaks the GC's # assumptions about what can be untracked. Make sure we re-track result # tuples whenever we reuse them. - i = iter({None: []}.items()) + it = iter({None: []}.items()) gc.collect() # That GC collection probably untracked the recycled internal result # tuple, which is initialized to (None, None). Make sure it's re-tracked # when it's mutated and returned from __next__: - self.assertTrue(gc.is_tracked(next(i))) + self.assertTrue(gc.is_tracked(next(it))) @support.cpython_only def test_dict_items_result_gc(self): # Same as test_dict_items_result_gc above, but reversed. - i = reversed({None: []}.items()) + it = reversed({None: []}.items()) gc.collect() - self.assertTrue(gc.is_tracked(next(i))) + self.assertTrue(gc.is_tracked(next(it))) class CAPITest(unittest.TestCase): diff --git a/Lib/test/test_enumerate.py b/Lib/test/test_enumerate.py index 17a26fedb29519..906bfc21a26aed 100644 --- a/Lib/test/test_enumerate.py +++ b/Lib/test/test_enumerate.py @@ -140,12 +140,12 @@ def test_enumerate_result_gc(self): # bpo-42536: enumerate's tuple-reuse speed trick breaks the GC's # assumptions about what can be untracked. Make sure we re-track result # tuples whenever we reuse them. - e = self.enum([[]]) + it = self.enum([[]]) gc.collect() # That GC collection probably untracked the recycled internal result # tuple, which is initialized to (None, None). Make sure it's re-tracked # when it's mutated and returned from __next__: - self.assertTrue(gc.is_tracked(next(e))) + self.assertTrue(gc.is_tracked(next(it))) class MyEnum(enumerate): pass diff --git a/Lib/test/test_itertools.py b/Lib/test/test_itertools.py index 6a44582ea1cc01..6fdabd08faed44 100644 --- a/Lib/test/test_itertools.py +++ b/Lib/test/test_itertools.py @@ -301,13 +301,13 @@ def test_combinations_result_gc(self): # bpo-42536: combinations's tuple-reuse speed trick breaks the GC's # assumptions about what can be untracked. Make sure we re-track result # tuples whenever we reuse them. - c = combinations([None, []], 1) - next(c) + it = combinations([None, []], 1) + next(it) gc.collect() # That GC collection probably untracked the recycled internal result # tuple, which has the value (None,). Make sure it's re-tracked when # it's mutated and returned from __next__: - self.assertTrue(gc.is_tracked(next(c))) + self.assertTrue(gc.is_tracked(next(it))) def test_combinations_with_replacement(self): cwr = combinations_with_replacement @@ -401,10 +401,10 @@ def test_combinations_with_replacement_tuple_reuse(self): def test_combinations_with_replacement_result_gc(self): # Same as test_combinations_result_gc above, but for # combinations_with_replacement. - c = combinations_with_replacement([None, []], 1) - next(c) + it = combinations_with_replacement([None, []], 1) + next(it) gc.collect() - self.assertTrue(gc.is_tracked(next(c))) + self.assertTrue(gc.is_tracked(next(it))) def test_permutations(self): self.assertRaises(TypeError, permutations) # too few arguments @@ -482,10 +482,10 @@ def test_permutations_tuple_reuse(self): @support.cpython_only def test_permutations_result_gc(self): # Same as test_combinations_result_gc above, but for permutations. - p = permutations([None, []], 1) - next(p) + it = permutations([None, []], 1) + next(it) gc.collect() - self.assertTrue(gc.is_tracked(next(p))) + self.assertTrue(gc.is_tracked(next(it))) def test_combinatorics(self): # Test relationships between product(), permutations(), @@ -1000,9 +1000,9 @@ def test_zip_longest_tuple_reuse(self): @support.cpython_only def test_zip_longest_result_gc(self): # Same as test_combinations_result_gc above, but for zip_longest. - z = zip_longest([[]]) + it = zip_longest([[]]) gc.collect() - self.assertTrue(gc.is_tracked(next(z))) + self.assertTrue(gc.is_tracked(next(it))) def test_zip_longest_pickling(self): for proto in range(pickle.HIGHEST_PROTOCOL + 1): @@ -1153,10 +1153,10 @@ def test_product_tuple_reuse(self): @support.cpython_only def test_product_result_gc(self): # Same as test_combinations_result_gc above, but for product. - p = product([None, []]) - next(p) + it = product([None, []]) + next(it) gc.collect() - self.assertTrue(gc.is_tracked(next(p))) + self.assertTrue(gc.is_tracked(next(it))) def test_product_pickling(self): # check copy, deepcopy, pickle diff --git a/Lib/test/test_ordered_dict.py b/Lib/test/test_ordered_dict.py index 63f47279c1b57e..eb404463e92550 100644 --- a/Lib/test/test_ordered_dict.py +++ b/Lib/test/test_ordered_dict.py @@ -705,12 +705,12 @@ def test_ordered_dict_items_result_gc(self): # bpo-42536: OrderedDict.items's tuple-reuse speed trick breaks the GC's # assumptions about what can be untracked. Make sure we re-track result # tuples whenever we reuse them. - i = iter(self.OrderedDict({None: []}).items()) + it = iter(self.OrderedDict({None: []}).items()) gc.collect() # That GC collection probably untracked the recycled internal result # tuple, which is initialized to (None, None). Make sure it's re-tracked # when it's mutated and returned from __next__: - self.assertTrue(gc.is_tracked(next(i))) + self.assertTrue(gc.is_tracked(next(it))) class PurePythonOrderedDictTests(OrderedDictTests, unittest.TestCase): From dfcb32ceed3c3add822f7324e49b7f6d07b54934 Mon Sep 17 00:00:00 2001 From: Brandt Bucher Date: Fri, 4 Dec 2020 10:27:48 -0800 Subject: [PATCH 21/23] Group new itertools tests --- Lib/test/test_itertools.py | 90 +++++++++++++++++++------------------- 1 file changed, 45 insertions(+), 45 deletions(-) diff --git a/Lib/test/test_itertools.py b/Lib/test/test_itertools.py index 6fdabd08faed44..a99b5e2bb71db9 100644 --- a/Lib/test/test_itertools.py +++ b/Lib/test/test_itertools.py @@ -296,19 +296,6 @@ def test_combinations_tuple_reuse(self): self.assertEqual(len(set(map(id, combinations('abcde', 3)))), 1) self.assertNotEqual(len(set(map(id, list(combinations('abcde', 3))))), 1) - @support.cpython_only - def test_combinations_result_gc(self): - # bpo-42536: combinations's tuple-reuse speed trick breaks the GC's - # assumptions about what can be untracked. Make sure we re-track result - # tuples whenever we reuse them. - it = combinations([None, []], 1) - next(it) - gc.collect() - # That GC collection probably untracked the recycled internal result - # tuple, which has the value (None,). Make sure it's re-tracked when - # it's mutated and returned from __next__: - self.assertTrue(gc.is_tracked(next(it))) - def test_combinations_with_replacement(self): cwr = combinations_with_replacement self.assertRaises(TypeError, cwr, 'abc') # missing r argument @@ -397,15 +384,6 @@ def test_combinations_with_replacement_tuple_reuse(self): self.assertEqual(len(set(map(id, cwr('abcde', 3)))), 1) self.assertNotEqual(len(set(map(id, list(cwr('abcde', 3))))), 1) - @support.cpython_only - def test_combinations_with_replacement_result_gc(self): - # Same as test_combinations_result_gc above, but for - # combinations_with_replacement. - it = combinations_with_replacement([None, []], 1) - next(it) - gc.collect() - self.assertTrue(gc.is_tracked(next(it))) - def test_permutations(self): self.assertRaises(TypeError, permutations) # too few arguments self.assertRaises(TypeError, permutations, 'abc', 2, 1) # too many arguments @@ -479,14 +457,6 @@ def test_permutations_tuple_reuse(self): self.assertEqual(len(set(map(id, permutations('abcde', 3)))), 1) self.assertNotEqual(len(set(map(id, list(permutations('abcde', 3))))), 1) - @support.cpython_only - def test_permutations_result_gc(self): - # Same as test_combinations_result_gc above, but for permutations. - it = permutations([None, []], 1) - next(it) - gc.collect() - self.assertTrue(gc.is_tracked(next(it))) - def test_combinatorics(self): # Test relationships between product(), permutations(), # combinations() and combinations_with_replacement(). @@ -997,13 +967,6 @@ def test_zip_longest_tuple_reuse(self): ids = list(map(id, list(zip_longest('abc', 'def')))) self.assertEqual(len(dict.fromkeys(ids)), len(ids)) - @support.cpython_only - def test_zip_longest_result_gc(self): - # Same as test_combinations_result_gc above, but for zip_longest. - it = zip_longest([[]]) - gc.collect() - self.assertTrue(gc.is_tracked(next(it))) - def test_zip_longest_pickling(self): for proto in range(pickle.HIGHEST_PROTOCOL + 1): self.pickletest(proto, zip_longest("abc", "def")) @@ -1150,14 +1113,6 @@ def test_product_tuple_reuse(self): self.assertEqual(len(set(map(id, product('abc', 'def')))), 1) self.assertNotEqual(len(set(map(id, list(product('abc', 'def'))))), 1) - @support.cpython_only - def test_product_result_gc(self): - # Same as test_combinations_result_gc above, but for product. - it = product([None, []]) - next(it) - gc.collect() - self.assertTrue(gc.is_tracked(next(it))) - def test_product_pickling(self): # check copy, deepcopy, pickle for args, result in [ @@ -1620,6 +1575,51 @@ def test_StopIteration(self): self.assertRaises(StopIteration, next, f(lambda x:x, [])) self.assertRaises(StopIteration, next, f(lambda x:x, StopNow())) + @support.cpython_only + def test_combinations_result_gc(self): + # bpo-42536: combinations's tuple-reuse speed trick breaks the GC's + # assumptions about what can be untracked. Make sure we re-track result + # tuples whenever we reuse them. + it = combinations([None, []], 1) + next(it) + gc.collect() + # That GC collection probably untracked the recycled internal result + # tuple, which has the value (None,). Make sure it's re-tracked when + # it's mutated and returned from __next__: + self.assertTrue(gc.is_tracked(next(it))) + + @support.cpython_only + def test_combinations_with_replacement_result_gc(self): + # Ditto for combinations_with_replacement. + it = combinations_with_replacement([None, []], 1) + next(it) + gc.collect() + self.assertTrue(gc.is_tracked(next(it))) + + @support.cpython_only + def test_permutations_result_gc(self): + # Ditto for permutations. + it = permutations([None, []], 1) + next(it) + gc.collect() + self.assertTrue(gc.is_tracked(next(it))) + + @support.cpython_only + def test_product_result_gc(self): + # Ditto for product. + it = product([None, []]) + next(it) + gc.collect() + self.assertTrue(gc.is_tracked(next(it))) + + @support.cpython_only + def test_zip_longest_result_gc(self): + # Ditto for zip_longest. + it = zip_longest([[]]) + gc.collect() + self.assertTrue(gc.is_tracked(next(it))) + + class TestExamples(unittest.TestCase): def test_accumulate(self): From 06e05f4119fc59b71c7521e3d699a9a6d6af8fe8 Mon Sep 17 00:00:00 2001 From: Brandt Bucher Date: Fri, 4 Dec 2020 10:39:21 -0800 Subject: [PATCH 22/23] Beef up the NEWS entry --- .../2020-12-02-20-23-31.bpo-42536.Kx3ZOu.rst | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-) diff --git a/Misc/NEWS.d/next/Core and Builtins/2020-12-02-20-23-31.bpo-42536.Kx3ZOu.rst b/Misc/NEWS.d/next/Core and Builtins/2020-12-02-20-23-31.bpo-42536.Kx3ZOu.rst index c6efd6f85d7d78..f6b100f2390c38 100644 --- a/Misc/NEWS.d/next/Core and Builtins/2020-12-02-20-23-31.bpo-42536.Kx3ZOu.rst +++ b/Misc/NEWS.d/next/Core and Builtins/2020-12-02-20-23-31.bpo-42536.Kx3ZOu.rst @@ -1,4 +1,17 @@ Several built-in and standard library types now ensure that their internal -result tuples are always tracked by the garbage collector. Previously, they -could have become untracked by a prior garbage collection. Patch by Brandt -Bucher. +result tuples are always tracked by the :term:`garbage collector +`: + +- :meth:`collections.OrderedDict.items` +- :meth:`dict.items` +- :func:`enumerate` +- :func:`functools.reduce` +- :func:`itertools.combinations` +- :func:`itertools.combinations_with_replacement` +- :func:`itertools.permutations` +- :func:`itertools.product` +- :func:`itertools.zip_longest` +- :func:`zip` + +Previously, they could have become untracked by a prior garbage collection. +Patch by Brandt Bucher. From 60e75193e3d3a0fbb4e7d9b507a5969c35516fe0 Mon Sep 17 00:00:00 2001 From: Brandt Bucher Date: Fri, 4 Dec 2020 12:04:58 -0800 Subject: [PATCH 23/23] Fix doc build --- .../2020-12-02-20-23-31.bpo-42536.Kx3ZOu.rst | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/Misc/NEWS.d/next/Core and Builtins/2020-12-02-20-23-31.bpo-42536.Kx3ZOu.rst b/Misc/NEWS.d/next/Core and Builtins/2020-12-02-20-23-31.bpo-42536.Kx3ZOu.rst index f6b100f2390c38..6ccacab1f64f68 100644 --- a/Misc/NEWS.d/next/Core and Builtins/2020-12-02-20-23-31.bpo-42536.Kx3ZOu.rst +++ b/Misc/NEWS.d/next/Core and Builtins/2020-12-02-20-23-31.bpo-42536.Kx3ZOu.rst @@ -2,15 +2,24 @@ Several built-in and standard library types now ensure that their internal result tuples are always tracked by the :term:`garbage collector `: -- :meth:`collections.OrderedDict.items` +- :meth:`collections.OrderedDict.items() ` + - :meth:`dict.items` + - :func:`enumerate` + - :func:`functools.reduce` + - :func:`itertools.combinations` + - :func:`itertools.combinations_with_replacement` + - :func:`itertools.permutations` + - :func:`itertools.product` + - :func:`itertools.zip_longest` + - :func:`zip` Previously, they could have become untracked by a prior garbage collection.