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:
- #458
- cloudpipe/cloudpickle#320

changelog.d/847.change.rst

@@ -0,0 +1,2 @@
+Attrs classes are now fully compatible with cloudpickle.
+`#847 <https://github.com/python-attrs/attrs/issues/847>`_

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",

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()

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__
 

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)

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.