Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix closing of callbacks on CLI exit #2680

Merged
merged 2 commits into from
Nov 3, 2024

Conversation

kdeldycke
Copy link
Contributor

@kdeldycke kdeldycke commented Feb 23, 2024

This PR forces the context to close itself on CLI exits.

This makes sure all callbacks registered by invoked options are properly called whenever a ctx.exit() action is triggered. Which has the effect of preventing state leaks.

These leaks cannot be witness in standard CLI operations, because on ctx.exit() call, the OS process that is running the Python CLI will be destroyed.

Still, in unittests, these state leaks will leads to flaky and non-deterministic tests, as the pytest, tox or any other test framework will keep using the same Python interpreter. To demonstrate this, I implemented a unit test using loggers as a fixture, in addition to a generic unit test.

Checklist:

  • Add tests that demonstrate the correct behavior of the change. Tests should fail without the change.
  • Add or update relevant docs, in the docs folder and in code.
  • Add an entry in CHANGES.rst summarizing the change and linking to the issue.
  • Add .. versionchanged:: entries in any relevant code docs.
  • Run pre-commit hooks and fix any issues.
  • Run pytest and tox, no tests failed.

@kdeldycke kdeldycke force-pushed the context-close-before-exit branch from 10197fd to b292dc5 Compare February 23, 2024 12:45
kdeldycke added a commit to kdeldycke/click-extra that referenced this pull request Feb 23, 2024
@kdeldycke
Copy link
Contributor Author

Note that this might be related to: #1053

@kdeldycke kdeldycke force-pushed the context-close-before-exit branch from b292dc5 to fb6b6e7 Compare May 18, 2024 12:26
@aenglander aenglander added this to the 8.2.0 milestone May 20, 2024
@AndreasBackx AndreasBackx mentioned this pull request Oct 20, 2024
34 tasks
[pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

Add changelog entry

Lint
@AndreasBackx AndreasBackx force-pushed the context-close-before-exit branch from fb6b6e7 to 32f6f50 Compare November 3, 2024 12:24
@AndreasBackx AndreasBackx merged commit c326df9 into pallets:main Nov 3, 2024
12 checks passed
@kdeldycke kdeldycke deleted the context-close-before-exit branch November 3, 2024 14:31
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 20, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants