mainloop: make periodic events restartable

When we shut down tor, we disable periodic events, but leave their enabled flag set to 1.

See this PR for details: https://github.com/torproject/tor/pull/1397

I'm not sure if this is the best solution, or how far we should backport.

changed milestone to %Tor: 0.3.5.x-final

added 035-backport 040-backport 041-backport 042-backport 042-should BugSmashFund actualpoints::0.1 component::core tor/tor consider-backport-after-0424 milestone::Tor: 0.3.5.x-final owner::dgoulet points::0.5 priority::high regression? resolution::fixed reviewer::nickm severity::normal status::closed type::defect version::tor 0.3.4.1-alpha labels

Hmmmm so indeed periodic_event_disconnect() doesn't unset the enabled flag which is periodic_event_disable() job.

It appears that when tearing down tor, we call a "disconnect all" but that doesn't cancel any of the event, it just disconnect them which free() the libevent event without event_del() before (which is the periodic_event_disable() job to do).

I'm starting to think that before a disconnect, there should be a disable so the event can be freed and its enabled flag set to 0?

Trac:
Reviewer: N/A to dgoulet

I agree that doing "disable" as part of disconnect is fine.

event_free() already does event_del(), so that part is okay.

Trac:
Priority: Medium to High

035 PR: https://github.com/torproject/tor/pull/1409 041+ PR: https://github.com/torproject/tor/pull/1410

Approach here was to disable the event before disconnecting it when tearing down everything.

This code is well known by nickm and already commented on the approach. Setting nickm as the Reviewer.

Trac:
Actualpoints: N/A to 0.1
Status: assigned to needs_review
Reviewer: dgoulet to nickm

Looks valid, but I would like to suggest a change for future safety.

Let's do one of the following:

• Document, in periodic_event_disconnect(), that you must first call periodic_event_disable().
• Make periodic_event_disconnect() itself call periodic_event_disable(), so that we can't forget to do so.

Trac:
Status: needs_review to needs_revision

Replying to nickm:

• Make periodic_event_disconnect() itself call periodic_event_disable(), so that we can't forget to do so.

I prefer this approach.

I've updated 035 PR with a fixup commit. With this, it merges nicely forward up to master :) (as in PR 1410 can be ignored now).

PR: ​https://github.com/torproject/tor/pull/1409 Branch: `ticket32058_035_01

Trac:
Status: needs_revision to needs_review

Okay, made a squashed branch as ticket32058_035_01_squashed and a PR at https://github.com/torproject/tor/pull/1422 . When CI is done, we can merge to 0.4.2 and forward, then mark for backport.

Trac:
Status: needs_review to merge_ready

Merged to 0.4.2 and later, leaving open for backport.

Trac:
Keywords: N/A deleted, consider-backport-after-0424 added
Milestone: Tor: 0.4.2.x-final to Tor: 0.4.1.x-final

Merged to 0.3.5 and later.

Merged #32575 (moved), #31939 (moved), #31548 (moved), #30344 (moved), #30258 (moved), #28970 (moved), #31091 (moved), and #32108 (moved) together.

Trac:
Milestone: Tor: 0.4.1.x-final to Tor: 0.3.5.x-final
Status: merge_ready to closed
Resolution: N/A to fixed

closed

changed time estimate to 4h

added 48m of time spent

moved to tpo/core/tor#32058 (closed)