Notify systemd of ShutdownWaitLength #417
Conversation
Pull Request Test Coverage Report for Build 2792
💛 - Coveralls |
src/feature/hibernate/hibernate.c
Outdated
| * ShutdownWaitLength to more than that, but use a longer type just in case | ||
| */ | ||
| sd_notifyf(0, "EXTEND_TIMEOUT_USEC=%" PRIu64, | ||
| (uint64_t)(options->ShutdownWaitLength + 30) * TOR_USEC_PER_SEC); |
There was a problem hiding this comment.
ShutdownWaitLength is an int. To avoid (a very unlikely) overflow on addition, use:
| (uint64_t)(options->ShutdownWaitLength + 30) * TOR_USEC_PER_SEC); | |
| ((uint64_t)(options->ShutdownWaitLength) + 30) * TOR_USEC_PER_SEC); |
There was a problem hiding this comment.
I am not sure why you are giving tor an extra 30 seconds beyond the shutdown wait length to shut down:
https://www.freedesktop.org/software/systemd/man/systemd.service.html#TimeoutStopSec=
1s or 2s seems like a reasonable time for tor to run its event loop and exit, but 30s might make relay operators think that their device has hung.
There was a problem hiding this comment.
I made it a very conservative number because I don't know how long tor might actually take, and because the default is 90 seconds. In fact, it might make more sense to use 90, because that's the default, and ShutdownWaitLength is a sort of "added time". Also note that systemd won't wait around forever like an idiot; as long as tor actually exits before this time the shutdown process will continue immediately. The only difference is that if tor actually fails to exit for some reason then systemd will wait for 30 seconds longer. Also, this won't do anything with the default configurations for ShutdownWaitLength and DefaultTimeoutStopSec, because tor will tell systemd to wait for 30+30=60 seconds, but systemd will wait min(60, DefaultTimeoutStopSec=90)=90 seconds.
There was a problem hiding this comment.
For future reference, I think you should've opened two separate comments here (I think that's possible...).
There was a problem hiding this comment.
Oh, I remember why I increased this from my first revision: I wanted to consider the case where tor may in fact take an extra few tens of seconds to clean up, which is not entirely absurd, like if we try to free memory that was actually swapped out because it was used at startup and then never again.
There was a problem hiding this comment.
I made it a very conservative number because I don't know how long tor might actually take, and because the default is 90 seconds. In fact, it might make more sense to use 90, because that's the default, and ShutdownWaitLength is a sort of "added time". Also note that systemd won't wait around forever like an idiot; as long as tor actually exits before this time the shutdown process will continue immediately. The only difference is that if tor actually fails to exit for some reason then systemd will wait for 30 seconds longer. Also, this won't do anything with the default configurations for ShutdownWaitLength and DefaultTimeoutStopSec, because tor will tell systemd to wait for 30+30=60 seconds, but systemd will wait min(60, DefaultTimeoutStopSec=90)=90 seconds.
I understand now. Please add a comment that explains the systemd default of 90 seconds.
changes/ticket28113
Outdated
| @@ -0,0 +1,3 @@ | |||
| o Minor features (relay usability): | |||
| - Notify systemd of ShutdownWaitLength so it can be set to more than 90 | |||
There was a problem hiding this comment.
The Tor default is actually 30s:
https://github.com/torproject/tor/blob/master/contrib/dist/tor.service.in
| - Notify systemd of ShutdownWaitLength so it can be set to more than 90 | |
| - Notify systemd of ShutdownWaitLength so it can be set to more than 30 |
There was a problem hiding this comment.
Yes, but the systemd default is 90.
There was a problem hiding this comment.
The default in the tor systemd config is 30 seconds. This overrides the systemd default. So on distributions that use the tor systemd config, this patch is required to extend ShutdownWaitLength beyond 30 seconds.
To avoid relay operator confusion, let's use the lower figure.
There was a problem hiding this comment.
Hm... to be honest I'm not sure how the default TimeoutSec works. It seems to me like if tor waits 30 seconds, and then does some non-zero work, then it should always be killed by systemd, assuming that systemd is not overly busy.
…pires This commit upstreams the Debian package setting of 60 seconds for TimeoutStopSec, but applies it to startup and shutdown. Part of 28113.
No description provided.