From d7615cc8846ffb4b0105c62735401855d9fd7d41 Mon Sep 17 00:00:00 2001 From: paul fisher Date: Fri, 29 Oct 2021 15:12:39 -0400 Subject: [PATCH] Pull thread-local into _compat module to fix cloudpickling. Because cloudpickle tries to pickle a function's globals, when it pickled an attrs instance, it would try to pickle the `__repr__` method and its globals, which included a `threading.local`. This broke cloudpickle for all attrs classes unless they explicitly specified `repr=False`. Modules, however, are pickled by reference, not by value, so moving the repr into a different module means we can put `_compat` into the function's globals and not worry about direct references. Includes a test to ensure that attrs and cloudpickle remain compatible. Also adds an explanation of the reason we even *have* that global thread-local variable. It wasn't completely obvious to a reader why the thread-local was needed to track reference cycles in `__repr__` calls, and the test did not previously contain a cycle that touched a non-attrs value. This change adds a comment explaining the need and tests a cycle that contains non-attrs values. Fixes: - https://github.com/python-attrs/attrs/issues/458 - https://github.com/cloudpipe/cloudpickle/issues/320 --- changelog.d/847.change.rst | 2 ++ setup.py | 2 ++ src/attr/_compat.py | 15 ++++++++++ src/attr/_make.py | 56 ++++++++++++++++++------------------- tests/test_compatibility.py | 26 +++++++++++++++++ tests/test_dunders.py | 17 +++++++++++ 6 files changed, 89 insertions(+), 29 deletions(-) create mode 100644 changelog.d/847.change.rst create mode 100644 tests/test_compatibility.py diff --git a/changelog.d/847.change.rst b/changelog.d/847.change.rst new file mode 100644 index 000000000..c4657bffd --- /dev/null +++ b/changelog.d/847.change.rst @@ -0,0 +1,2 @@ +Attrs classes are now fully compatible with cloudpickle. +`#847 `_ diff --git a/setup.py b/setup.py index 79e45bfda..0314ba007 100644 --- a/setup.py +++ b/setup.py @@ -47,6 +47,8 @@ EXTRAS_REQUIRE = { "docs": ["furo", "sphinx", "zope.interface", "sphinx-notfound-page"], "tests_no_zope": [ + # For regression test to ensure cloudpickle compat doesn't break. + "cloudpickle", # 5.0 introduced toml; parallel was broken until 5.0.2 "coverage[toml]>=5.0.2", "hypothesis", diff --git a/src/attr/_compat.py b/src/attr/_compat.py index cc20246cb..9d03ac196 100644 --- a/src/attr/_compat.py +++ b/src/attr/_compat.py @@ -2,6 +2,7 @@ import platform import sys +import threading import types import warnings @@ -243,3 +244,17 @@ def func(): set_closure_cell = make_set_closure_cell() + +# Thread-local global to track attrs instances which are already being repr'd. +# This is needed because there is no other (thread-safe) way to pass info +# about the instances that are already being repr'd through the call stack +# in order to ensure we don't perform infinite recursion. +# +# For instance, if an instance contains a dict which contains that instance, +# we need to know that we're already repr'ing the outside instance from within +# the dict's repr() call. +# +# This lives here rather than in _make.py so that the functions in _make.py +# don't have a direct reference to the thread-local in their globals dict. +# If they have such a reference, it breaks cloudpickle. +repr_context = threading.local() diff --git a/src/attr/_make.py b/src/attr/_make.py index 9dba4e835..c2a92cef3 100644 --- a/src/attr/_make.py +++ b/src/attr/_make.py @@ -4,12 +4,13 @@ import inspect import linecache import sys -import threading import warnings from operator import itemgetter -from . import _config, setters +# We need to import _compat itself in addition to the _compat members to avoid +# having the thread-local in the globals here. +from . import _compat, _config, setters from ._compat import ( HAS_F_STRINGS, PY2, @@ -1864,8 +1865,6 @@ def _add_eq(cls, attrs=None): return cls -_already_repring = threading.local() - if HAS_F_STRINGS: def _make_repr(attrs, ns, cls): @@ -1883,7 +1882,7 @@ def _make_repr(attrs, ns, cls): for name, r, _ in attr_names_with_reprs if r != repr } - globs["_already_repring"] = _already_repring + globs["_compat"] = _compat globs["AttributeError"] = AttributeError globs["NOTHING"] = NOTHING attribute_fragments = [] @@ -1908,24 +1907,23 @@ def _make_repr(attrs, ns, cls): else: cls_name_fragment = ns + ".{self.__class__.__name__}" - lines = [] - lines.append("def __repr__(self):") - lines.append(" try:") - lines.append(" working_set = _already_repring.working_set") - lines.append(" except AttributeError:") - lines.append(" working_set = {id(self),}") - lines.append(" _already_repring.working_set = working_set") - lines.append(" else:") - lines.append(" if id(self) in working_set:") - lines.append(" return '...'") - lines.append(" else:") - lines.append(" working_set.add(id(self))") - lines.append(" try:") - lines.append( - " return f'%s(%s)'" % (cls_name_fragment, repr_fragment) - ) - lines.append(" finally:") - lines.append(" working_set.remove(id(self))") + lines = [ + "def __repr__(self):", + " try:", + " already_repring = _compat.repr_context.already_repring", + " except AttributeError:", + " already_repring = {id(self),}", + " _compat.repr_context.already_repring = already_repring", + " else:", + " if id(self) in already_repring:", + " return '...'", + " else:", + " already_repring.add(id(self))", + " try:", + " return f'%s(%s)'" % (cls_name_fragment, repr_fragment), + " finally:", + " already_repring.remove(id(self))", + ] return _make_method( "__repr__", "\n".join(lines), unique_filename, globs=globs @@ -1954,12 +1952,12 @@ def __repr__(self): Automatically created by attrs. """ try: - working_set = _already_repring.working_set + already_repring = _compat.repr_context.already_repring except AttributeError: - working_set = set() - _already_repring.working_set = working_set + already_repring = set() + _compat.repr_context.already_repring = already_repring - if id(self) in working_set: + if id(self) in already_repring: return "..." real_cls = self.__class__ if ns is None: @@ -1979,7 +1977,7 @@ def __repr__(self): # for the duration of this call, it's safe to depend on id(...) # stability, and not need to track the instance and therefore # worry about properties like weakref- or hash-ability. - working_set.add(id(self)) + already_repring.add(id(self)) try: result = [class_name, "("] first = True @@ -1993,7 +1991,7 @@ def __repr__(self): ) return "".join(result) + ")" finally: - working_set.remove(id(self)) + already_repring.remove(id(self)) return __repr__ diff --git a/tests/test_compatibility.py b/tests/test_compatibility.py new file mode 100644 index 000000000..0dab852ec --- /dev/null +++ b/tests/test_compatibility.py @@ -0,0 +1,26 @@ +""" +Tests for compatibility against other Python modules. +""" + +import cloudpickle + +from hypothesis import given + +from .strategies import simple_classes + + +class TestCloudpickleCompat(object): + """ + Tests for compatibility with ``cloudpickle``. + """ + + @given(simple_classes()) + def test_repr(self, cls): + """ + attrs instances can be pickled and un-pickled with cloudpickle. + """ + inst = cls() + # Exact values aren't a concern so long as neither direction + # raises an exception. + pkl = cloudpickle.dumps(inst) + cloudpickle.loads(pkl) diff --git a/tests/test_dunders.py b/tests/test_dunders.py index 3a8300399..57d33bef1 100644 --- a/tests/test_dunders.py +++ b/tests/test_dunders.py @@ -367,6 +367,23 @@ class Cycle(object): cycle.cycle = cycle assert "Cycle(value=7, cycle=...)" == repr(cycle) + def test_infinite_recursion_long_cycle(self): + """ + A cyclic graph can pass through other non-attrs objects, and repr will + still emit an ellipsis and not raise an exception. + """ + + @attr.s + class LongCycle(object): + value = attr.ib(default=14) + cycle = attr.ib(default=None) + + cycle = LongCycle() + # Ensure that the reference cycle passes through a non-attrs object. + # This demonstrates the need for a thread-local "global" ID tracker. + cycle.cycle = {"cycle": [cycle]} + assert "LongCycle(value=14, cycle={'cycle': [...]})" == repr(cycle) + def test_underscores(self): """ repr does not strip underscores.