From c3f63ac1432044271b15c4ef7959000a7a0d9bfe Mon Sep 17 00:00:00 2001
From: Bruno Oliveira <nicoddemus@gmail.com>
Date: Wed, 13 Dec 2017 19:37:45 -0200
Subject: [PATCH] Fix memory leak caused by fixture values never been garbage
 collected

The leak was caused by the (unused) `FixtureRequest._fixture_values`
cache.

This was introduced because the `partial` object (created to call
FixtureDef.finish() bound with the Request) is kept alive
through the entire session when a function-scoped fixture depends
on a session-scoped (or higher) fixture because of the nested
`addfinalizer` calls.

FixtureDef.finish() started receiving a request object in order to
obtain the proper hook proxy object (#2127), but this does not seem
useful at all in practice because `pytest_fixture_post_finalizer`
will be called with the `request` object of the moment the fixture value
was *created*, not the request object active when the fixture value
is being destroyed. We should probably deprecate/remove the request
parameter from `pytest_fixture_post_finalizer`.

Fix #2981
---
 _pytest/fixtures.py        | 17 ++++++++-------
 changelog/2981.bugfix      |  1 +
 testing/acceptance_test.py | 43 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 54 insertions(+), 7 deletions(-)
 create mode 100644 changelog/2981.bugfix

diff --git a/_pytest/fixtures.py b/_pytest/fixtures.py
index e09ffaddb..a7a63f505 100644
--- a/_pytest/fixtures.py
+++ b/_pytest/fixtures.py
@@ -267,7 +267,6 @@ class FixtureRequest(FuncargnamesCompatAttr):
         self.fixturename = None
         #: Scope string, one of "function", "class", "module", "session"
         self.scope = "function"
-        self._fixture_values = {}  # argname -> fixture value
         self._fixture_defs = {}  # argname -> FixtureDef
         fixtureinfo = pyfuncitem._fixtureinfo
         self._arg2fixturedefs = fixtureinfo.name2fixturedefs.copy()
@@ -450,8 +449,7 @@ class FixtureRequest(FuncargnamesCompatAttr):
                 raise
         # remove indent to prevent the python3 exception
         # from leaking into the call
-        result = self._getfixturevalue(fixturedef)
-        self._fixture_values[argname] = result
+        self._compute_fixture_value(fixturedef)
         self._fixture_defs[argname] = fixturedef
         return fixturedef
 
@@ -466,7 +464,14 @@ class FixtureRequest(FuncargnamesCompatAttr):
             values.append(fixturedef)
             current = current._parent_request
 
-    def _getfixturevalue(self, fixturedef):
+    def _compute_fixture_value(self, fixturedef):
+        """
+        Creates a SubRequest based on "self" and calls the execute method of the given fixturedef object. This will
+        force the FixtureDef object to throw away any previous results and compute a new fixture value, which
+        will be stored into the FixtureDef object itself.
+
+        :param FixtureDef fixturedef:
+        """
         # prepare a subrequest object before calling fixture function
         # (latter managed by fixturedef)
         argname = fixturedef.argname
@@ -515,12 +520,11 @@ class FixtureRequest(FuncargnamesCompatAttr):
         exc_clear()
         try:
             # call the fixture function
-            val = fixturedef.execute(request=subrequest)
+            fixturedef.execute(request=subrequest)
         finally:
             # if fixture function failed it might have registered finalizers
             self.session._setupstate.addfinalizer(functools.partial(fixturedef.finish, request=subrequest),
                                                   subrequest.node)
-        return val
 
     def _check_scope(self, argname, invoking_scope, requested_scope):
         if argname == "request":
@@ -573,7 +577,6 @@ class SubRequest(FixtureRequest):
         self.scope = scope
         self._fixturedef = fixturedef
         self._pyfuncitem = request._pyfuncitem
-        self._fixture_values = request._fixture_values
         self._fixture_defs = request._fixture_defs
         self._arg2fixturedefs = request._arg2fixturedefs
         self._arg2index = request._arg2index
diff --git a/changelog/2981.bugfix b/changelog/2981.bugfix
new file mode 100644
index 000000000..fd6dde37a
--- /dev/null
+++ b/changelog/2981.bugfix
@@ -0,0 +1 @@
+Fix **memory leak** where objects returned by fixtures were never destructed by the garbage collector.
diff --git a/testing/acceptance_test.py b/testing/acceptance_test.py
index 9da9091db..4480fc2cf 100644
--- a/testing/acceptance_test.py
+++ b/testing/acceptance_test.py
@@ -913,3 +913,46 @@ def test_deferred_hook_checking(testdir):
     })
     result = testdir.runpytest()
     result.stdout.fnmatch_lines(['* 1 passed *'])
+
+
+def test_fixture_values_leak(testdir):
+    """Ensure that fixture objects are properly destroyed by the garbage collector at the end of their expected
+    life-times (#2981).
+    """
+    testdir.makepyfile("""
+        import attr
+        import gc
+        import pytest
+        import weakref
+
+        @attr.s
+        class SomeObj(object):
+            name = attr.ib()
+
+        fix_of_test1_ref = None
+        session_ref = None
+
+        @pytest.fixture(scope='session')
+        def session_fix():
+            global session_ref
+            obj = SomeObj(name='session-fixture')
+            session_ref = weakref.ref(obj)
+            return obj
+
+        @pytest.fixture
+        def fix(session_fix):
+            global fix_of_test1_ref
+            obj = SomeObj(name='local-fixture')
+            fix_of_test1_ref = weakref.ref(obj)
+            return obj
+
+        def test1(fix):
+            assert fix_of_test1_ref() is fix
+
+        def test2():
+            gc.collect()
+            # fixture "fix" created during test1 must have been destroyed by now
+            assert fix_of_test1_ref() is None
+    """)
+    result = testdir.runpytest()
+    result.stdout.fnmatch_lines(['* 2 passed *'])