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 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

@aenglander aenglander added this to the 8.2.0 milestone May 20, 2024
@AndreasBackx AndreasBackx mentioned this pull request Oct 20, 2024
39 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 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants