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

Force terminate apps if shutdown.timeout_seconds is defined, without requiring shutdown.command to be defined #214

Closed
anrddh opened this issue Aug 6, 2024 · 6 comments
Labels
done Done, awaiting release enhancement New feature or request

Comments

@anrddh
Copy link
Contributor

anrddh commented Aug 6, 2024

Feature Request

Use Case:

I drive a number of processes that don't reliably shutdown on SIGTERM. Usually, this is down to buggy behavior on the part of those processes. It would be nice if Process Compose was robust to such processes, and provided a way to kill them reliably.

Proposed Change:

If shutdown.timeout_seconds is defined for a process, then SIGKILL it after that many seconds, if it hasn't shutdown by then.

Who Benefits From The Change(s)?

Anyone driving processes that don't shutdown gracefully reliably.

Alternative Approaches

We can use pkill -9 in shutdown.command to get by for now, but this feels like a bit of a hack. It requires care to ensure processes running outside Process Compose with similar names don't get accidentally killed.

@anrddh
Copy link
Contributor Author

anrddh commented Aug 6, 2024

If this seems like a reasonable feature, then I am happy to look into the source code and try and come up with a fix for this myself.

@F1bonacc1
Copy link
Owner

Hi @anrddh,

That sounds like a valid use case and a very relevant feature.

@F1bonacc1 F1bonacc1 added the enhancement New feature or request label Aug 9, 2024
@anrddh
Copy link
Contributor Author

anrddh commented Aug 19, 2024

Here's a rough cut of the change: main...anrddh:process-compose:forced-termination

I've never written Go before, so please do look carefully! It seems to pass a simple test. (This is not a great unit test BTW and could be flaky if run on machines under heavy load, but I don't know of a straightforward way to sidestep the calls to sleep.)

I ended up using command contexts because it seemed to be the best way to avoid kills racing with waiting for the dead process.

Longer term, I think it would be nice if we could support cgroups on Linux to handle all kinds of edge cases properly.

I'm about to head to bed, but I'll look tomorrow and refine the change further before opening a PR.

@anrddh
Copy link
Contributor Author

anrddh commented Aug 28, 2024

I've been kept busy the last few days, but my branch above fails one of the tests. I know what the issue is, and I'll try and get it up to snuff by the weekend

@F1bonacc1
Copy link
Owner

Added in v1.27.0

Thank you @anrddh for taking the time to contribute to this project. I appreciate your effort and interest in improving the codebase. While I've decided to go in a different direction for this feature, your engagement is valuable and I hope you'll continue to be involved with the project.

Cheers!

@anrddh
Copy link
Contributor Author

anrddh commented Oct 19, 2024

Thanks for implementing it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
done Done, awaiting release enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants