Finalizer re-registered in parent fixture even when the value of the fixture is cached

[jakkdl]

When getting the value of a fixture, it will always add its finalizer to parent fixtures, regardless of if it's cached or not. When we have several subfixtures, this leads to unpredictable ordering between them on when they're torn down (if triggered by the parent fixture being torn down). It is also an obvious inefficiency, where a long-lived parent fixture could rack up tons of irrelevant fixtures.

As found in discussion of #11833, which resolved a similar issue. Also related to #4871

repro

import pytest


@pytest.fixture(scope="module")
def fixture_1(request):
    ...

@pytest.fixture(scope="module")
def fixture_2(fixture_1):
    print("setup 2")
    yield
    print("teardown 2")

@pytest.fixture(scope="module")
def fixture_3(fixture_1):
    print("setup 3")
    yield
    print("teardown 3")

def test_1(fixture_2):
    ...
def test_2(fixture_3):
    ...

# This will add a second copy of fixture_2's finalizer in the parent fixture, causing it to
# be torn down before fixture 3.
def test_3(fixture_2):
    ...


# Trigger finalization of fixture_1, otherwise the cleanup would sequence 3&2 before 1 as normal.
@pytest.mark.parametrize("fixture_1", [None], indirect=["fixture_1"])
def test_4(fixture_1):
    ...

output

setup 2
setup 3
teardown 2
teardown 3

but if we remove test_3 we get 2-3-3-2.

suggested fix

Here's the relevant method:

pytest/src/_pytest/fixtures.py

Lines 1037 to 1079 in 2607fe8

	def execute(self, request: SubRequest) -> FixtureValue:
	finalizer = functools.partial(self.finish, request=request)
	# Get required arguments and register our own finish()
	# with their finalization.
	for argname in self.argnames:
	fixturedef = request._get_active_fixturedef(argname)
	if not isinstance(fixturedef, PseudoFixtureDef):
	fixturedef.addfinalizer(finalizer)
	my_cache_key = self.cache_key(request)
	if self.cached_result is not None:
	cache_key = self.cached_result[1]
	# note: comparison with `==` can fail (or be expensive) for e.g.
	# numpy arrays (#6497).
	if my_cache_key is cache_key:
	if self.cached_result[2] is not None:
	exc = self.cached_result[2]
	raise exc
	else:
	result = self.cached_result[0]
	return result
	# We have a previous but differently parametrized fixture instance
	# so we need to tear it down before creating a new one.
	self.finish(request)
	assert self.cached_result is None
	ihook = request.node.ihook
	try:
	# Setup the fixture, run the code in it, and cache the value
	# in self.cached_result
	result = ihook.pytest_fixture_setup(fixturedef=self, request=request)
	finally:
	# schedule our finalizer, even if the setup failed
	request.node.addfinalizer(finalizer)
	return result
	def cache_key(self, request: SubRequest) -> object:
	return request.param_index if not hasattr(request, "param") else request.param
	def __repr__(self) -> str:
	return f"<FixtureDef argname={self.argname!r} scope={self.scope!r} baseid={self.baseid!r}>"

I thought this would be a trivial fix, merely moving the code that adds finalizers to parent fixtures (L1041 to L1044) to after the check on whether the value is cached (L1062). But this broke tests in very weird ways... and I think that's the same problem as noted in #4871 (comment) - see "Possible solution - make the cache key not match". Current behaviour requires that the call to request._get_active_fixturedef(argname) happens before checking if the value is cached, to let the parent fixture check if it has been differently parametrized -> if so tear itself down -> tear us down -> invalidate our cache.

Once I figured out that the finalize-adding code had dual purposes, it was fairly easy to split it and achieve the wanted behavior without breaking anything:

    def execute(self, request: SubRequest) -> FixtureValue:
        # Ensure arguments (parent fixtures) are loaded. If their cache has been
        # invalidated they will also finalize us and invalidate our cache.
        # If/when parent fixture parametrization is included in our cache key
        # this can be moved after checking our cache key.
        parent_fixtures_to_add_finalizer = []
        for argname in self.argnames:
            fixturedef = request._get_active_fixturedef(argname)
            if not isinstance(fixturedef, PseudoFixtureDef):
                # save fixture as one to add our finalizer to, if we're not cached
                # resolves #12135
                parent_fixtures_to_add_finalizer.append(fixturedef)

        my_cache_key = self.cache_key(request)
        if self.cached_result is not None:
            ...

        finalizer = functools.partial(self.finish, request=request)
        # add finalizer to parent fixtures
        for parent_fixture in parent_fixtures_to_add_finalizer:
            parent_fixture.addfinalizer(finalizer)

possible bad outcomes

... I have a very hard time coming up with any downside of implementing this. Will write a PR once I've rewritten the repro as a proper regression test.

[bluetech]

... I have a very hard time coming up with any downside of implementing this

If fixture_2 is already cached, then we know that it's already registered in fixture_1's finalizer stack, and we know fixture_1 hasn't switched params otherwise it would have finalized and finished fixture_2 as well (so it wouldn't be cached). So the only effect is to "bump" fixture_2 order in the fixture_1's finalizer stack. In this example, the effect is bad. Is there any case where it's needed for correctness? I don't think so -- if there is a dependency, it would have already been reflected.

So I agree with your conclusion. Good catch!