Opened 5 weeks ago

Last modified 4 weeks ago

#32058 merge_ready defect

mainloop: make periodic events restartable

Reported by: teor Owned by: dgoulet
Priority: High Milestone: Tor: 0.4.1.x-final
Component: Core Tor/Tor Version: Tor: 0.3.4.1-alpha
Severity: Normal Keywords: consider-backport-after-0424, 042-should, regression?, 035-backport, 040-backport, 041-backport, 042-backport, BugSmashFund
Cc: nickm Actual Points: 0.1
Parent ID: Points: 0.5
Reviewer: nickm Sponsor:

Description

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.

Child Tickets

Change History (8)

comment:1 Changed 5 weeks ago by dgoulet

Reviewer: dgoulet

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?

comment:2 Changed 5 weeks ago by nickm

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

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

comment:3 Changed 5 weeks ago by nickm

Priority: MediumHigh

comment:4 Changed 5 weeks ago by dgoulet

Actual Points: 0.1
Reviewer: dgouletnickm
Status: assignedneeds_review

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.

comment:5 Changed 5 weeks ago by nickm

Status: needs_reviewneeds_revision

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.

comment:6 in reply to:  5 Changed 5 weeks ago by dgoulet

Status: needs_revisionneeds_review

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

comment:7 Changed 5 weeks ago by nickm

Status: needs_reviewmerge_ready

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.

comment:8 Changed 4 weeks ago by teor

Keywords: consider-backport-after-0424 added
Milestone: Tor: 0.4.2.x-finalTor: 0.4.1.x-final

Merged to 0.4.2 and later, leaving open for backport.

Note: See TracTickets for help on using tickets.