Merge pull request #29 from njsmith/no-refcycle-in-unwrap

njsmith · web-flow · commit 4b917c462d1b · 2020-11-15T20:34:37.000-08:00
Avoid creating a reference cycle when calling Error.unwrap
diff --git a/src/outcome/_impl.py b/src/outcome/_impl.py
@@ -134,7 +134,22 @@ def unwrap(self):
         # Tracebacks show the 'raise' line below out of context, so let's give
         # this variable a name that makes sense out of context.
         captured_error = self.error
-        raise captured_error
+        try:
+            raise captured_error
+        finally:
+            # We want to avoid creating a reference cycle here. Python does
+            # collect cycles just fine, so it wouldn't be the end of the world
+            # if we did create a cycle, but the cyclic garbage collector adds
+            # latency to Python programs, and the more cycles you create, the
+            # more often it runs, so it's nicer to avoid creating them in the
+            # first place. For more details see:
+            #
+            #    https://github.com/python-trio/trio/issues/1770
+            #
+            # In particuar, by deleting this local variables from the 'unwrap'
+            # methods frame, we avoid the 'captured_error' object's
+            # __traceback__ from indirectly referencing 'captured_error'.
+            del captured_error, self
 
     def send(self, it):
         self._set_unwrapped()
diff --git a/tests/test_sync.py b/tests/test_sync.py
@@ -1,3 +1,4 @@
+import sys
 import traceback
 
 import pytest
@@ -117,3 +118,22 @@ def raise_ValueError(x):
     frames = traceback.extract_tb(exc_info.value.__traceback__)
     functions = [function for _, _, function, _ in frames]
     assert functions[-2:] == ['unwrap', 'raise_ValueError']
+
+
+def test_Error_unwrap_does_not_create_reference_cycles():
+    # See comment in Error.unwrap for why reference cycles are tricky
+    exc = ValueError()
+    err = Error(exc)
+    try:
+        err.unwrap()
+    except ValueError:
+        pass
+    # Top frame in the traceback is the current test function; we don't care
+    # about its references
+    assert exc.__traceback__.tb_frame is sys._getframe()
+    # The next frame down is the 'unwrap' frame; we want to make sure it
+    # doesn't reference the exception (or anything else for that matter, just
+    # to be thorough)
+    unwrap_frame = exc.__traceback__.tb_next.tb_frame
+    assert unwrap_frame.f_code.co_name == "unwrap"
+    assert unwrap_frame.f_locals == {}
