jasonacox / Powerwall-Dashboard

Grafana Monitoring Dashboard for Tesla Solar and Powerwall Systems
MIT License
270 stars 57 forks source link

python servers (pypowerwall and weather411) do not stop gracefully when container is stopped #353

Closed rcasta74 closed 9 months ago

rcasta74 commented 9 months ago

I'm using podman so I don't know if it is the same with docker.

When I run stop command with podman, a SIGTERM signal is sent to the main process of the container, but it looks like that this signal is not handled properly within pypowerwall and weather411 containers since they do not stop and are killed after 10 seconds of timeout. If I send them SIGINT signal instead, they stop gracefully.

mcbirse commented 9 months ago

Hey @rcasta74 - I can confirm the same issue occurs under docker.

I noticed this recently also, as I am currently working on adapting my tesla-history script that runs under docker (for the Solar Only users) for another purpose (probably will be another tool addition), and realised it is also affected by this issue, and does not gracefully shutdown when you stop the docker container.

I added code very similar to your fix, as below:

# Capture SIGTERM to exit gracefully
def sig_exit(signum, frame):
    raise SystemExit
signal.signal(signal.SIGTERM, sig_exit)

Essentially the same as your fix, since sys.exit() will raise SystemExit.

I decided on using raise SystemExit as it seemed clearer that the program then should have a try...except block in the main routine/loop to capture the exception to exit gracefully. People may not realise sys.exit() raises SystemExit (I didn't, until looking into this).

rcasta74 commented 9 months ago

@mcbirse thanks for confirming that the issue occurs under docker as well, and for explaining your choise of code. Let @jasonacox choose which code he prefers, no matter if my PRs are merged or not :)

jasonacox commented 9 months ago

Great find! I'll invite @mcbirse as the reviewer of the PR. Can one of you submit the raise SystemExit recommendation edit?