runner: correctly cleanup item _request/funcargs if an exception was reraised during call (e.g. KeyboardInterrupt)

[mon]

In my test suite, I have some objects that rely on garbage collection to be cleaned up correctly. This is not amazing, but it's how the code is structured for now. If I interrupt tests with Ctrl+C, or by manually raising KeyboardInterrupt in a test, these objects are not cleaned up any more.

call_and_report re-raises Exit and KeyboardInterrupt, which breaks the cleanup logic in runtestprotocol that unsets the item funcargs (which is where my objects end up living as references, as they're passed in as fixtures).

By just wrapping the entire block with try: ... finally: ..., cleanup works again as expected.

I'm going to hold off on updating changelog/AUTHORS until this PR is accepted!

Template checklist things:

• [n/a] Include documentation when adding new features.
• Include new tests or update existing tests when applicable (hard to test CPython reference/GC behaviour, suggestions appreciated).
• Allow maintainers to push and squash when merging my commits. Please uncheck this if you prefer to squash the commits yourself.

If this change fixes an issue, please:

• [n/a] Add text like closes #XYZW to the PR description and/or commits

Unless your change is trivial or a small documentation fix (e.g., a typo or reword of a small section) please:

• Create a new changelog file in the changelog folder, with a name like <ISSUE NUMBER>.<TYPE>.rst. See changelog/README.rst for details.

• Add yourself to AUTHORS in alphabetical order.

[RonnyPfannschmidt]

We also need test that demonstrates the problem and that it was fixed by this change

In general the suggestion is to never ever use gc for Ressource management

But test tools need to account for their users dealing with legacy codebases that have that misbehavior

[mon]

Alright, I'll try and make a test case to validate the fix. Does it otherwise seem OK?

The real reason for the GC-based cleanup is because session scoped test fixtures are being too aggressively cleaned up by pytest, but that is a much harder problem for me to get my head around and presumably harder to actually solve. So here I am relying on Cpython specifics...

[RonnyPfannschmidt]

If you trade session scope with dunder del something is very wrong

Its a use case we explicitly condone

Its on the don't do and if you had to migrate away from it ASAP list

[mon]

I'll revisit the reason that session scopes fixtures aren't working then, sounds like a significantly better long-term solution. I think it was something along the lines of #8102 (I am parametrizing these session fixtures).

[RonnyPfannschmidt]

Sounds like you want a retaining factory fixture

[mon]

I appreciate you taking the time to help, this __del__ based cleanup has been bothering me since I convinced myself I needed it. I'm not entirely sure what a retaining factory fixture is, could you could give me an example to plug into my tests?

If context helps, here's a minimum repro of my issue. Looking at the --setup-plan output, because my session fixtures are a cartesian product and pytest seems to handle fixture cleanup on a per-test basis, I always get fixtures setup and finalised more than once. In this example it's just S1, but it scales with the number of tests.

conftest.py

import pytest

@pytest.fixture(scope="session")
def client(): pass
@pytest.fixture(scope="session")
def server(): pass

class C1: pass
class C2: pass
class S1: pass
class S2: pass

def pytest_generate_tests(metafunc: pytest.Metafunc):
    metafunc.parametrize("client", (C1, C2), scope="session", indirect=True)
    metafunc.parametrize("server", (S1, S2), scope="session", indirect=True)

test_test.py

def test_teardown_order1(client, server): pass
def test_teardown_order2(client, server): pass

[RonnyPfannschmidt]

In the posted case multiple setups/teardowns are expexted as only one parameter variant can be active at a time

A retaining fixture would be a non parameterized fixture one is3s as factory/ final cleanup

Setup would then as the factory for a instance

Ans teardowns would entirely be left to the factory

That way the parameterized fixtures would be reduced helpers that as the factory foe a instance

[mon]

I think I understand, I'll try and get something working over the next few days. Will it roughly look like this?

@pytest.fixture(scope="session")
def factory():
    active = {}

    def fn(param: type):
        if existing := active.get(param):
            return existing
        
        active[param] = param()
        return active[param]

    yield fn

    for x in active.values():
        x.cleanup()

@pytest.fixture
def client(factory, request):
    yield factory(request.param)

[RonnyPfannschmidt]

@mon my understanding is that you actual/final goal would be more along the lines of having a parameterized global fixture of which more than one instance can be active at the same time and thus partially excluding it from the test reordering problems

[mon]

I didn't quite understand "of which more than one instance can be active at the same time" since that was my problem to begin with, so I ended up going with my previous solution and it's worked great!

Is it worth closing this PR or is it a useful goal to clean things up regardless?

[RonnyPfannschmidt]

Lifecycle management is hard currently we can't express the case of multiple can be active in the internal datastructures

This is a artifacts of how the fixture system organically evolved between 2010 and 2016

[bluetech]

Regardless of the very true comments by @RonnyPfannschmidt (prefer to not rely on __del__), I do think this change makes sense, i.e. that the cleanup is supposed to happen also on Ctrl-C.

Writing a proper semantic regression test for this is hard because even Cpython doesn't guarantee "release on last reference" semantics. So I wrote a simple unit test that the _request is cleared instead. Also added a changelog entry.

[mon]

Fantastic to hear, thank you so much!