Make it clear in the documentation that fixtures are not suitable for managing resource lifecycles

[gsmethells]

What's the problem this feature will solve?

Partitioned off of #5243

Proposal: Make it clear in the pytest documentation that users are responsible for resource lifecycle management.

Currently, there are misleading examples such as:

@pytest.fixture(scope="module", params=["smtp.gmail.com", "mail.python.org"])
def smtp_connection(request):
    smtp_connection = smtplib.SMTP(request.param, 587, timeout=5)
    yield smtp_connection
    print("finalizing {}".format(smtp_connection))
    smtp_connection.close()

that suggest otherwise. The above fixture from the documentation is highly suggestive that pytest believes fixtures are appropriate to use for resource management; however, pytest is only a test runner and not a resource manager according to @asottile

Describe the solution you'd like

A documentation change that makes it clear that pytest fixtures are not fit for the purpose of handling resource lifecycles in all cases (user beware). There are likely other places in the documentation that require review concerning this point.

Also, it would be useful to all users to document common ways a user can handle resource lifecycles, especially for external resources that could be leaked, that are known to work smoothly with pytest's framework.

Alternative Solutions

None

Additional context

Identified during discussions in:

• Finalizers don't run on SIGTERM #5243

[nicoddemus]

Hi @gsmethells,

Not sure I agree, "not suitable for managing resource lifecycles" is a bit strong worded. For example even if we handled SIGTERM as commented in #5243, a crash in the process would still leave dangling resources.

I'm OK with adding a note that SIGTERM is not handled thought.

[gsmethells]

SIGSEGV would be a pretty dire case that is beyond any software to handle and is also a bit strongly worded rebuke to the proposal.

Fixtures have no built-in failsafe regarding resource leaks.

That is the essence of the necessary change to the documentation. Users should have fair warning that fixtures cannot handle:

• SIGINT
• SIGTERM
• SIGQUIT

However:

1. SIGTERM is in common use by CI runners like GitLab, Jenkins, et al when they cancel a CI job
2. pytest is only a test runner and not a resource manager according to @asottile

Therefore, I believe it is a fair assessment, given both those facts, that "fixtures are not fit for the purpose of resource lifecycle management." Users are deserving of a warning of this subtle issue that they are likely to run into eventually.

SIGSEGV goes without saying and does not belong in the discussion.

[asottile]

@gsmethells I recommend you take a step back and consider how you're communicating -- we understand you're passionate about this issue but you don't need to use inflammatory language to get your point across. please be nice -- there are humans on the other side of the cable

as is standard in python, SIGINT is handled (via raising KeyboardInterrupt) which calls finalizers / teardowns / etc. -- you'll recall my recommendation from the original thread was to transform SIGTERM into SIGINT to extend that behaviour if you want it

[gsmethells]

@asottile understood

[gsmethells]

I seek to help other users than myself. I believe the best place for it, in this case, is in the documentation given the stances I learned by spending my time on these threads.

The suggested solution would need further refinement to suss out issues for more complicated test suite codebases. It doesn't work for us. That can be figured out in #9142 at some point.

Until then, I hope you will agree that the current documentation is not definitive concerning where the responsibility of resource lifecycle management rests and how best to go about it. I lay it out as I do in a proof format more for the formality of my thoughts than any intention to be unkind and I am sorry if it was taken otherwise. If it is any consolation, I spent a good part of my waking hours being useful regarding the Sept 30th Let's Encrypt's snafu for hundreds of clinics in every state of the U.S. (some of the largest in the country) and I'm pretty spent, but that's no excuse.

[nicoddemus]

Until then, I hope you will agree that the current documentation is not definitive concerning where the responsibility of resource lifecycle management rests and how best to go about it.

I agree, how about something like:

pytest does not do any special processing for `SIGTERM` and `SIGQUIT` signals (`SIGINT` is handled naturally by the Python runtime via `KeyboardInterrupt`), so fixtures that manage external resources which are important
to be cleared when the Python process is terminated (by those signals) might leak resources.

If fixtures in your suite need special care regarding termination in those scenarios, consider using the pytest-XXX plugin.

My intent here is explicitly warn about the signals not handled by pytest, and provide an alternative when handling those signals is important.

[nicoddemus]

@asottile out of curiosity, what drawbacks you see in making pytest handle SIGTERM to stop executing and perform teardown (like we do with KeyboardInterrupt)? I think listing the drawbacks explicitly helps explaining to users why pytest doesn't handle them.

(I'm wondering about concrete points, not necessarily about the definition/terminology of "resource manager" vs "test runner")

[asottile]

signal handlers are global and if pytest starts adding them that will interfere with execution of current code in a surprising way (I know specifically of at least one private project that breaks due to testing signal handlers directly)

[nicoddemus]

Ahh makes sense, thanks!

[gsmethells]

@nicoddemus I'm good with the above blurb. Thanks for taking the time to write it up.

[gsmethells]

@asottile thanks for taking the time to explain why signal handlers do not belong in pytest core.

[nicoddemus]

PR open: #9150