notify systemd if shutdown will be longer than 30 seconds

currently systemd just kills tor if the user sets ShutdownWaitLength more than 30 seconds. we should tell systemd not to kill tor.

changed milestone to %Tor: unspecified in legacy/trac

added 033-unreached-backport-maybe in Legacy / Trac 034-unreached-backport-maybe in Legacy / Trac 035-backport-maybe in Legacy / Trac component::core tor/tor in Legacy / Trac milestone::Tor: unspecified in Legacy / Trac owner::Hello71 in Legacy / Trac priority::medium in Legacy / Trac reviewer-was-teor-20190422 in Legacy / Trac severity::normal in Legacy / Trac status::needs-revision in Legacy / Trac tor-systemd in Legacy / Trac type::defect in Legacy / Trac version::tor 0.2.6.2-alpha in Legacy / Trac labels

https://github.com/torproject/tor/pull/417

Trac:
Status: assigned to needs_review

This has been a bug since we introduced systemd support in 0.2.6.2-alpha (or possibly 0.2.6.3-alpha).

It's a simple patch, so I think we might want to backport it. Tenatively assigning it to 0.3.5.

Trac:
Version: Tor: unspecified to Tor: 0.2.6.2-alpha
Milestone: N/A to Tor: 0.3.5.x-final
Keywords: N/A deleted, 034-backport-maybe, 029-backport-maybe, tor-systemd, 033-backport-maybe, 035-backport-maybe added

I reviewed the pull request: a minor refactor and a comment would be helpful.

Trac:
Reviewer: N/A to teor
Status: needs_review to needs_revision

Trac:
Status: needs_revision to needs_review

Failing Travis. Easy fix, on the changes file:

./changes/ticket28113:
	Missing subcategory on 'Minor features'

Please see my comments on the pull request.

We should test this pull request in an alpha before backporting it.

Trac:
Status: needs_review to needs_revision

(It's pretty safe, because it only extends the systemd timeout. But we should check it doesn't extend the timeout too much.)

I'd recommend that if we do backport, we backport no farther than 0.3.5.

Replying to teor:

Please see my comments on the pull request.

Replied there.

We should test this pull request in an alpha before backporting it.

Agreed.

Replying to teor:

(It's pretty safe, because it only extends the systemd timeout. But we should check it doesn't extend the timeout too much.)

As I explained on the PR, it doesn't matter too much if the number sent is too high, since systemd will continue immediately as long as tor actually exits before the timeout (it's a maximum limit, not a minimum or exact). If tor doesn't exit expeditiously, then that's a bug in tor. IMO it's worth trying to work around the (still hypothetical AFAIK) bug by waiting a little longer, rather than crashing and almost certainly corrupting cache files. Also, I figure nobody will care excessively if they set ShutdownWaitLength to 2 minutes and tor actually takes 2 minutes 15 seconds to quit, as long as we don't accidentally make it 20 minutes (but again, that would require another separate bug in tor).

Trac:
Status: needs_revision to needs_review

Thanks for these changes.

Let's fix the changes file, and add an extra comment explaining the default systemd timeout.

Trac:
Status: needs_review to needs_revision

I pushed three small commits to your pull request to fix these issues:

1. If tor's systemd TimeoutStopSec is less than the ShutdownWaitLength, plus the time it takes tor to shut down after its ShutdownWaitLength has expired, then systemd may terminate Tor abnormally. Let's explain that better in the changes file.

2. Tor's systemd file in dist/ has a TimeoutSec of 30, which is too low: https://gitweb.torproject.org/tor.git/tree/contrib/dist/tor.service.in Debian has a TimeoutStopSec of 60, which seems like a good change to upstream: https://gitweb.torproject.org/debian/tor.git/tree/debian/systemd/tor@default.service

3. This is a bugfix, so it should get a bugfix changes file

See my branch ticket28113-029 for the squash and cherry-pick to 0.2.9: https://github.com/torproject/tor/pull/474

This branch merges cleanly all the way to master: https://github.com/torproject/tor/pull/475 (GitHub's pull requests can't do this merge, but git can.)

Trac:
Status: needs_revision to merge_ready

Merging to 0.3.5, marking for possible backport.

Trac:
Milestone: Tor: 0.3.5.x-final to Tor: 0.3.4.x-final

Hm. Apparently this broke with systemd-devel installed. 212bd977 made it work for me, but I'm a little worried about how tested this could be.

Replying to nickm:

Hm. Apparently this broke with systemd-devel installed. 212bd977 made it work for me, but I'm a little worried about how tested this could be.

I didn't test with libsystemd-devel, and travis doesn't have libsystemd-devel installed.

So I guess that just leaves any testing that Hello71 did.

I use Arch which includes devel files in the standard package. It turns out, though, that I either accidentally removed the patch from my local list or never included it in the first place, so it would seem that I haven't tested this at all. I may have assumed that Travis had systemd-devel already. Patch is already applied though, and seems like too much work to back it out, so let's say test it for a little while and see if anyone complains. :P

Trac:
Status: merge_ready to needs_information

unfortunately it seems that it does not actually work, I still get tor killed.

Trac:
Status: needs_information to needs_revision

This ticket still needs changes and testing.

Trac:
Milestone: Tor: 0.3.4.x-final to Tor: unspecified

These open, non-merge_ready tickets can not get backported to 0.3.3, because 0.3.3 is now unsupported.

Hmm, I guess they should still get 033-backport-unreached

Trac:
Keywords: N/A deleted, 033-backport-unreached added

If these tickets go back in to needs_review, and I am on leave, they will need another reviewer.

Trac:
Keywords: N/A deleted, reviewer-was-teor-20190422 added
Reviewer: teor to N/A

Is this still a bug? Maybe somebody should open a Debian bug, and the systemd script there can get fixed?

well the code was committed, and as far as I can tell it just doesn't work. I haven't had time to figure out why, and I doubt I'll have time in the near to medium term future.

I think the systemd service doesn't require any (more) fixing. as far as I know, the TimeoutStopSec is sufficient in all distributions now. the problem is just that if you increase ShutdownWaitLength (in tor), you may need to also increase TimeoutStopSec in systemd. this is really a nice-to-have though; the code can be removed and this ticket closed if nobody wants to work on it.

Removing 034-backport from all open tickets: 034 has reached EOL.

0.3.4 is unsupported, we won't be backporting anything else to it.

Trac:
Keywords: 034-backport-maybe deleted, 034-unreached-backport-maybe added

Fix 033-unreached-backport spelling.

Trac:
Keywords: 033-backport-unreached deleted, 033-unreached-backport added

Remove outdated 033-backport-maybe

Trac:
Keywords: 033-backport-maybe deleted, 033-unreached-backport-maybe added

Trac:
Keywords: 033-unreached-backport deleted, N/A added

0.2.9 is no longer maintained as of 1 January 2020. Therefore, we won't be backporting anything to it.

(None of these tickets are merge_ready, so we can just delete the backport tag.)

0.2.9 is no longer maintained as of 1 January 2020. Therefore, we won't be backporting anything to it.

(None of these tickets are merge_ready, so we can just delete the 029-backport-maybe tag.)

Trac:
Keywords: 029-backport-maybe deleted, N/A added

mentioned in issue legacy/trac#28195 (moved)

mentioned in issue legacy/trac#28378 (moved)

moved from legacy/trac#28113 (moved)

added Bug label and removed 1 deleted label

added 035-backport + 1 deleted label and removed 1 deleted label

added First Contribution label

no activity in 9 months; unassigning. Please feel free to re-assign if you want to work on this again.

unassigned @Hello71

removed milestone

added Icebox label

added Close in favor of Arti label

closed