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.

Sign up for GitHub

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

Jump to bottom

Click doesn't close file options during shell completion #2644

Closed
gregorias opened this issue Dec 3, 2023 · 5 comments · Fixed by #2800
Closed

Click doesn't close file options during shell completion #2644

gregorias opened this issue Dec 3, 2023 · 5 comments · Fixed by #2800
Assignees
@AndreasBackx
Labels
bug f:completion feature: shell completion

Comments

@gregorias
Copy link

gregorias commented Dec 3, 2023 •
edited
Loading

Click doesn't close file options during shell completion, which causes a resource warning if a program uses a file option.

For example, I have group like this:

@click.group()
@click.option('--config_file',
              default=CONFIG,
              type=click.File(mode='r'),
              help='help')
@click.pass_context
def cli(ctx, config_file: typing.TextIO):

and I get this warning:

/Users/grzesiek/Library/Caches/pypoetry/virtualenvs/findata-fetcher-3hK6JJJX-py3.12/lib/python3.12/site-packages/click/shell_completion.py:293: ResourceWarning: unclosed file <_io.TextIOWrapper name='/Users/grzesiek/.config/findata/fetcher.json' mode='r' encoding='UTF-8'>
  completions = self.get_completions(args, incomplete)

Details

I don't come with reproduction steps, but I can give something equally valuable, I can explain how this bug comes to be.

The issue stems from allocating a context in core.py outside of a with statement during shell completion. Here's a stack-trace of how that happens:

  File "/Users/grzesiek/.local/bin/findata-fetcher", line 8, in <module>
    sys.exit(main())
  File "/Users/grzesiek/.local/pipx/venvs/findata-fetcher/lib/python3.12/site-packages/fetcher/tool.py", line 576, in main
    cli(obj={})
  File "/Users/grzesiek/.local/pipx/venvs/findata-fetcher/lib/python3.12/site-packages/click/core.py", line 1171, in __call__
    return self.main(*args, **kwargs)
  File "/Users/grzesiek/.local/pipx/venvs/findata-fetcher/lib/python3.12/site-packages/click/core.py", line 1084, in main
    self._main_shell_completion(extra, prog_name, complete_var)
  File "/Users/grzesiek/.local/pipx/venvs/findata-fetcher/lib/python3.12/site-packages/click/core.py", line 1165, in _main_shell_completion
    rv = shell_complete(self, ctx_args, prog_name, complete_var,
  File "/Users/grzesiek/.local/pipx/venvs/findata-fetcher/lib/python3.12/site-packages/click/shell_completion.py", line 49, in shell_complete
    echo(comp.complete())
  File "/Users/grzesiek/.local/pipx/venvs/findata-fetcher/lib/python3.12/site-packages/click/shell_completion.py", line 296, in complete
    completions = self.get_completions(args, incomplete)
  File "/Users/grzesiek/.local/pipx/venvs/findata-fetcher/lib/python3.12/site-packages/click/shell_completion.py", line 273, in get_completions
    ctx = _resolve_context(self.cli, self.ctx_args, self.prog_name, args)
  File "/Users/grzesiek/.local/pipx/venvs/findata-fetcher/lib/python3.12/site-packages/click/shell_completion.py", line 513, in _resolve_context
    ctx = cli.make_context(prog_name, args.copy(), **ctx_args)
  File "/Users/grzesiek/.local/pipx/venvs/findata-fetcher/lib/python3.12/site-packages/click/core.py", line 952, in make_context
    with ctx.scope(cleanup=False):

This context gets returned to get_completions, but get_completions or its caller never call the context's __exit__ function.

Calling the __exit__ function is essential, because types.File depends on it for cleanup:

click/src/click/types.py

Line 740 in ca5e1c3

ctx.call_on_close(safecall(f.close))
.

Environment

  • Python version: 3.12
  • Click version: 8.1.7
akhil-vempali added a commit to akhil-vempali/click that referenced this issue Aug 21, 2024
@akhil-vempali
[FIX] fixes pallets#2644 pallets#2644
e65efc3
@davidism davidism mentioned this issue Aug 21, 2024
Closed
AndreasBackx added a commit that referenced this issue Nov 3, 2024
@AndreasBackx
Attempt at reproducing #2644.
cc2c24c
@AndreasBackx AndreasBackx mentioned this issue Nov 3, 2024
Merged
@AndreasBackx
Copy link
Collaborator

@gregorias, if you have some time. Could you create a test case that can reproduce the issue? I'm having a hard time exactly reproducing the error that you're seeing. Doesn't have to be a test case, but even an example CLI and what to call it with would be sufficient. Then I can look into this.

@AndreasBackx AndreasBackx added bug f:completion feature: shell completion labels Nov 3, 2024
@AndreasBackx AndreasBackx self-assigned this Nov 3, 2024
@gregorias
Copy link
Author

gregorias commented Nov 3, 2024 •
edited
Loading

You can find the repro repo + instructions at https://github.com/gregorias/click-2644. It’s crucial to enable the dev mode to see warnings with PYTHONDEVMODE.

@AndreasBackx
Copy link
Collaborator

@gregorias thank you so much. I'll give it a look. I have some experience with the resource warnings and the dev mode settings from at work where we're also doing some things around it.

AndreasBackx added a commit that referenced this issue Nov 3, 2024
@AndreasBackx
Attempt at reproducing #2644.
0f82ca4
@AndreasBackx
Copy link
Collaborator

AndreasBackx commented Nov 3, 2024 •
edited
Loading

It seems that the reason why I couldn't get it to repro was because when the file (default="README.md" in this case) does not exist, it doesn't open it. It does when it exists resulting in the warning. I'll change my test to incorporate a mocked/isolated filesystem. Thanks for the repro!

AndreasBackx added a commit that referenced this issue Nov 3, 2024
@AndreasBackx @akhil-vempali
Close contexts created during shell completion #2644. (#2800)
0ef55fd 
Co-authored-by: akhil-vempali <[email protected]>
@AndreasBackx
Copy link
Collaborator

Signing off for today. I haven't had the time to try and confirm your example is fixed now too, but it likely should be. I used it as an inspiration for the new test case. Let me know if you still see an issue! Thanks once again for the repro!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@AndreasBackx AndreasBackx

Labels
bug f:completion feature: shell completion
Projects
None yet
Milestone
No milestone
2 participants
@gregorias @AndreasBackx