Opened 7 months ago

Closed 7 months ago

#25762 closed defect (implemented)

Make periodic events array with flags including when they are enabled/disabled

Reported by: dgoulet Owned by: dgoulet
Priority: Medium Milestone: Tor: 0.3.4.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: 034-roadmap-subtask, client, s8-perf
Cc: Actual Points:
Parent ID: #25500 Points:
Reviewer: nickm Sponsor: Sponsor8

Description

This is part of #25500 bigger ticket.

The goal here is to put the periodic events (not the per-second callbacks) into an array of objects where an object has flags and enable/disable switch.

So far, the flags would indicate conditions on when to run the callbacks. One of them would be "do not run if disable network is on".

The object also needs to tell for which "tor entity" the callbacks applies. For instance, a certain callback would only apply to Client. Or a callback is only for Dirauth.

Probably more (or less) things will be added to the object as we start implementing.

Child Tickets

Change History (12)

comment:1 Changed 7 months ago by catalyst

Keywords: s8-perf added; performance removed

Normalize keywords.

comment:2 Changed 7 months ago by dgoulet

Alpha version: ticket25762_034_01.

This can probably be unit tested (at least for the double setup checks). Oh and it doesn't have yet "flags" to tell the setup process to run it or not like the "disable network". I'll do this once we agree on the interface.

Version 1, edited 7 months ago by dgoulet (previous) (next) (diff)

comment:3 Changed 7 months ago by nickm

I left some comments on the PR.

My main suggestion here would be:

  • Always initialize every periodic event. Instead of initializing only some...
  • Only call periodic_event_launch() on the events that should be running.
  • Add a periodic_event_disable() that delegates to mainloop_event_cancel(); call that on the events that should not be running.

comment:4 Changed 7 months ago by nickm

David asked me to look at his revised ticket25762_034_02. Looking better! I've left some suggested changes on my branch of the same name.

comment:5 Changed 7 months ago by dgoulet

Ok branch ticket25762_034_03 has the latest from Nickm with couple of tweaks from me including a fixup commit.

So imo, we are happy with the interface, this simply needs unit tests.

comment:6 Changed 7 months ago by nickm

This needs unit tests, and I also want to review these remaining things that I didn't check when I was reviewing the initial branch:

  • Did we accidentally "lose" any periodic callbacks?
  • Are all of the flags set correctly?

Also:

  • We should decide how this approach will handle events that turn off when DisableNetwork is set.

comment:7 in reply to:  6 Changed 7 months ago by dgoulet

Replying to nickm:

This needs unit tests, and I also want to review these remaining things that I didn't check when I was reviewing the initial branch:

  • Did we accidentally "lose" any periodic callbacks?

Yah... I think this means someone needs to go over commit f9370609e49e72d0 and make sure I didn't forget one :S.

  • Are all of the flags set correctly?

Indeed. Same commit from above. It is possible an event applies to a role I failed to notice...

Also:

  • We should decide how this approach will handle events that turn off when DisableNetwork is set.

I think we can do this part of #25376 and not within this branch.

comment:8 Changed 7 months ago by dgoulet

Reviewer: nickm
Status: assignedneeds_review

Ok unit tests added. I've rebased on latest master, merge fixup commits, cleaned up the commit list to make them nicer.

Branch: ticket25762_034_04
PR: https://github.com/torproject/tor/pull/56

comment:9 Changed 7 months ago by nickm

Status: needs_reviewneeds_revision

I had a couple of questions on some of the flags, but otherwise this looks good.

Once you've looked at the above questions, and made changes as appropriate, two final tests to do, if you haven't already:

  • Does it regress make test-network-all?
  • Does it cause any problems when you change flags on a running Tor instance and send it a SIGHUP?

comment:10 in reply to:  9 Changed 7 months ago by dgoulet

Status: needs_revisionneeds_review

Replying to nickm:

  • Does it regress make test-network-all?

It did! but I pushed a fixup commit.

  • Does it cause any problems when you change flags on a running Tor instance and send it a SIGHUP?

Looks fine with my testing!

The _04 branch has the fixup and the _05 branch has been rebased and squashed on latest master.

comment:11 Changed 7 months ago by nickm

great; I've merged the _05 branch to master!

comment:12 Changed 7 months ago by nickm

Resolution: implemented
Status: needs_reviewclosed
Note: See TracTickets for help on using tickets.