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: Propagate SIGINT to subprocess #75

Merged
merged 3 commits into from
Nov 29, 2023

Conversation

ReubenFrankel
Copy link
Contributor

@ReubenFrankel ReubenFrankel commented Mar 3, 2023

My knowledge of asyncio is quite limited but this seems to fix the issue for me. Please let me know if this can be optimised or there is more of a best-practice approach.

^CGracefully stopping... (press Ctrl+C again to force)
Aborting on container exit...
time="2023-03-03T01:12:55Z" level=error msg="got 3 SIGTERM/SIGINTs, forcing shutdown"
Container matatika-ext-app-1  Stopping
Container matatika-ext-app-1  Stopped
Container matatika-ext-catalog-1  Stopping
Container matatika-ext-catalog-1  Stopped
Container matatika-ext-db-1  Stopping
Container matatika-ext-db-1  Stopped
canceled


error invoking matatika None   error_message=Matatika invocation failed returncode=1

Could of unresolved minor issues:

  • Not sure what the got 3 SIGTERM/SIGINTs, forcing shutdown message is about - this doesn't happen when I Ctrl+C a docker compose up process directly
  • Usually the return code when you Ctrl+C a docker compose up process is 130, but here it is 1 (could be down to the above error)

Checklist

  • Implement fix to propagate SIGINT signal to asyncio subprocess
  • Add test(s)

Resolves #74


📚 Documentation preview 📚: https://meltano-edk--75.org.readthedocs.build/en/75/

@ReubenFrankel ReubenFrankel changed the title Propagate SIGINT to subprocess fix: Propagate SIGINT to subprocess Mar 3, 2023
@WillDaSilva WillDaSilva self-requested a review March 3, 2023 01:23
Copy link
Member

@WillDaSilva WillDaSilva left a comment

Choose a reason for hiding this comment

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

@ReubenFrankel Thanks you for this improvement. It looks good to me, but before we merge it in I'd appreciate the addition to a test to ensure it works, and to ensure there is no regression in this behaviour in the future.

@ReubenFrankel
Copy link
Contributor Author

ReubenFrankel commented Mar 27, 2023

@WillDaSilva I would be happy to do that. Is there an example I can follow somewhere already in the Meltano codebase? Just spent a while trying to adapt a copy of test_exec to fire a SIGINT signal to the mocked process, but have had no luck so far...

@WillDaSilva WillDaSilva merged commit 191b227 into meltano:main Nov 29, 2023
8 of 9 checks passed
@ReubenFrankel ReubenFrankel deleted the 74-propagate-sigint branch November 30, 2023 14:49
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.

Ctrl+C not passed to subprocesses properly
2 participants