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

Prevent softpack-core --help hanging forever #35

Merged
merged 2 commits into from
Jan 18, 2024
Merged

Conversation

sersorrel
Copy link
Member

The package list retrieval thread would previously run forever and prevent the interpreter from shutting down. This PR marks that thread as a daemon thread, so the interpreter will abruptly terminate it whenever the rest of the application exits (since all that thread does is "git clone into a temporary directory" and "run spack list", that's fine).

There was code in Application.main that was intended to prevent this from happening (by cancelling the thread once self.commands() returns), but Typer uses Click in "standalone mode" which means that self.commands() doesn't return, it just exits. So the main thread would exit, and the entire application would then wait for the package list retrieval thread to exit, but with no main thread to tell it to exit, it would run forever.

This prevents the thread from blocking application exit.

It also means the thread can be shut down abruptly, without resources
(like open files) being released, but that's ok – at most it will leave
a temporary directory lying around in /tmp. (And, in fact, it currently
never cleans up its temporary directories anyway.)
Typer runs Click commands in "standalone mode":

> Click will then handle exceptions and convert them into error messages
> and the function will never return but shut down the interpreter.

so any code after the call to Typer will never actually be executed.

The `assert False` is required to convince mypy that we know Typer will
never return.

Related: fastapi/typer#129
@sersorrel sersorrel requested a review from altaf-ali as a code owner January 18, 2024 15:03
Copy link

codecov bot commented Jan 18, 2024

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (13132b4) 65.37% compared to head (869a919) 65.29%.

Files Patch % Lines
softpack_core/spack.py 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop      #35      +/-   ##
===========================================
- Coverage    65.37%   65.29%   -0.09%     
===========================================
  Files           16       16              
  Lines          774      775       +1     
  Branches       131      131              
===========================================
  Hits           506      506              
- Misses         261      262       +1     
  Partials         7        7              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@mjkw31 mjkw31 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@sersorrel sersorrel merged commit 6d25b08 into develop Jan 18, 2024
3 of 5 checks passed
@sersorrel sersorrel deleted the fix/hang-on-exit branch January 18, 2024 16:17
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.

2 participants