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

Feature: Make kill signal configurable #871

Merged
merged 3 commits into from
Apr 29, 2024
Merged

Conversation

dadav
Copy link
Contributor

@dadav dadav commented Jan 10, 2024

This makes it possible to end exec commands gracefully.

Also fixed a typo on the way :)

Fixes #870

@dadav
Copy link
Contributor Author

dadav commented Jan 10, 2024

This also makes it possible to use gphoto2 as a source.

Like this:

exec:
  signal: sigint

streams:
  canon: exec:gphoto2 --capture-movie --stdout

@AlexxIT
Copy link
Owner

AlexxIT commented Jan 11, 2024

I don't think this is good idea to use same kill signal for all exec sources. It should be #param like ffmpeg and other sources.

@dadav
Copy link
Contributor Author

dadav commented Jan 11, 2024

I don't think this is good idea to use same kill signal for all exec sources. It should be #param like ffmpeg and other sources.

Good point, I'm gonna fix it

@skrashevich
Copy link
Contributor

The process may not terminating upon receipt of sigint. It's important to implement some timeout, after which sigkill will be sent. Otherwise, zombie processes may appear

@dadav
Copy link
Contributor Author

dadav commented Jan 11, 2024

Better? @AlexxIT

@skrashevich
Copy link
Contributor

skrashevich commented Jan 11, 2024

What happens if anyone set #killtimeout to zero or negative number? ;)

UPD: oops, looks like, that nothing bad

@dadav
Copy link
Contributor Author

dadav commented Jan 12, 2024

What happens if anyone set #killtimeout to zero or negative number? ;)

UPD: oops, looks like, that nothing bad

UPD: Negative or zero number will result in an immediate SIGKILL.

@dadav
Copy link
Contributor Author

dadav commented Jan 16, 2024

Is there anything I can work on to make this PR more acceptable?

@AlexxIT
Copy link
Owner

AlexxIT commented Jan 16, 2024

Sorry. Don't have time to review. Fixing my other projects after Home Assistant 2024.1 update...

@dadav
Copy link
Contributor Author

dadav commented Jan 21, 2024

Just realized this could make problems on windows/darwin. Need to invest some time in os specific code..

@dadav dadav force-pushed the signal branch 4 times, most recently from 5395454 to c566d05 Compare January 21, 2024 18:12
@dadav
Copy link
Contributor Author

dadav commented Jan 21, 2024

This should work..

@AlexxIT
Copy link
Owner

AlexxIT commented Apr 29, 2024

Thanks! I've rewritten the code to make it simpler.

@AlexxIT AlexxIT merged commit 732fe47 into AlexxIT:master Apr 29, 2024
@AlexxIT AlexxIT added this to the v1.9.0 milestone Apr 29, 2024
@felipecrs
Copy link
Contributor

I would probably change the default behavior, instead of killing by default, sending SIGTERM and waiting some arbitrary time like 5 seconds before killing the process.

@AlexxIT
Copy link
Owner

AlexxIT commented Apr 30, 2024

For example, ffmpeg will fail in any way, because stdout will be closed before kill signal.

@AlexxIT
Copy link
Owner

AlexxIT commented Apr 30, 2024

skrashevich added a commit to skrashevich/go2rtc that referenced this pull request May 1, 2024
This reverts commit 732fe47, reversing
changes made to d6774bb.
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.

Feature-Request: Make kill method of process configurable
4 participants