Update s3s.py to allow clean docker exit

[NaruZosa]

Process sigterm to allow the docker container to cleanly exit.

https://stackoverflow.com/questions/62853875/stopping-python-container-is-slow-sigterm-not-passed-to-python-process

"Docker runs your application, per default, in foreground, so, as PID 1, this said, the process with the PID 1 as a special meaning and specific protections in Linux.

This is highlighted in docker run documentation:

Note

A process running as PID 1 inside a container is treated specially by Linux: it ignores any signal with the default action. As a result, the process will not terminate on SIGINT or SIGTERM unless it is coded to do so."

[NaruZosa]

This is needed as Python doesn't process SIGTERM by default, so depending on OS, run method etc, sometimes Python will ignore shutdown signals.

This commit makes s3s process SIGTERM signals and terminate when instructed to do so.
I've had to do the same for my own Python projects (such as https://github.com/NaruZosa/salmon-run-notifier) to ensure they all process termination signals correctly, regardless of the architecture they're running on.

https://stackoverflow.com/questions/9930576/python-what-is-the-default-handling-of-sigterm

[ADeeeee]

Just curious. Which command do you use that having exit 1? I haven't got any while using docker run and only -r for s3s.py

[NaruZosa]

It's not an exit code of 1, rather the process has an ID of 1 in Docker.
It's not responding to SIGTERM, only SIGKILL.

You've probably noticed that it takes 10 seconds to stop when you use docker stop, that's because docker stop sends a SIGTERM, grants a 10 second grace period, and if it still hasn't stopped, sends a SIGKILL.

Try adding -t 60, such as docker stop s3s -t 60 and you'll notice it takes 60 seconds instead of 10 to stop, as it will send the SIGTERM, wait 60 seconds, the container still hasn't stopped so it sends a SIGKILL which stops it immediately.

This is an issue that affects all Python programs, and the impact isn't limited to Docker.

[ADeeeee]

Ah, my bad. Pardon me.

Actually I just run docker run --rm in my crontab for this script, so I don't start and stop it because it's not a daemon in the background at all.

[NaruZosa]

No worries.

The impact of the issue is that the script will never stop gracefully and must always be killed, which can cause it to stop while writing to the config files and corrupting them (happened to me a few times) and some operating systems won't be able to stop the container normally.

For example, Unraid sends a docker stop with a custom timeout when stopping a container from the GUI, and will raise an error if it has to resort to killing the container (or sometimes won't even reach the point of killing, so you need to drop to the command line). Relevant code for Unraid at https://github.com/unraid/webgui/blob/4f47afb7a837fcdb7806b330e0755c57836e0208/emhttp/plugins/dynamix.docker.manager/include/DockerClient.php#L783

[frozenpandaman]

Is there a way you can just make it so s3s doesn't get assigned PID 1? If you're saying this is an issue with Python itself, is it under discussion to be fixed with the next release/version of the language?

s3s does not support Docker and that's not the recommended way to run it (see #78 (comment), #67, #136) so I'm a bit hesitant to add this in. It doesn't really seem like it's on our end to handle either, but rather Python as a whole, as you mentioned. s3s (and the game) doesn't get updates anymore except for very small tweaks for the NSO app & SplatNet getting updated. Can you not just maintain this change locally?

[NaruZosa]

Sadly it's one of those things that's a 'feature' rather than a bug. Python provides hooks to process signals (via the signal module), and this includes SIGTERM. Due to that, it doesn't process SIGTERM like most other languages do out of the box, likely as that would remove the option to customise the handling of SIGTERM.
Because of that, to have SIGTERM handled correctly across operating systems, you need to add a handler that takes the SIGTERM and performs the action that is wanted (which this commit does).

I understand that Docker isn't officially supported, but was hoping that as this is an issue that can affect other platforms as well, that you will accept this pull request to make the termination logic universal regardless of the OS running s3s. If you'd prefer the code be moved to a separate module to make it tidier (such as handle_sigterm.py) and not in the main code, I'm happy to do this.

If you'd prefer not to accept pull requests and would like me to fork the project under the GPLv3 license instead, I can do this.

Regardless, I just want to say I'm really grateful for you for making s3s. I use it on a near daily basis, and know 3 other people IRL who use it very regularly as well, it's a fantastic tool, thank you so much :)

[NaruZosa]

For the sake of simplicity, theoretically this would work as an alternative:
signal.signal(signal.SIGTERM, sys.exit(0))

With no need for the terminate function introduced in this pull request.