Opened 3 months ago

Last modified 2 months ago

#31634 needs_revision enhancement

Check .may_include order and tor subsystem init order are compatible

Reported by: teor Owned by: nickm
Priority: Medium Milestone: Tor: 0.4.3.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: diagnostics, 043-should, practracker
Cc: gaba Actual Points:
Parent ID: Points: 2
Reviewer: teor Sponsor: Sponsor31-can

Description

In #31615, we discovered that our module init order doesn't match their dependency order.

Let's use practracker and tor to make sure that doesn't happen again. We'll probably need a new make check target, because we'll need a compiled tor and practracker output. See #31615 for details.

Gaba, this is Sponsor 31-can, because it helps us catch refactoring bugs when we create new modules.

Child Tickets

Change History (9)

comment:1 Changed 3 months ago by teor

We might also want to check against the order in subsystem_list.c.

comment:2 in reply to:  1 ; Changed 3 months ago by nickm

Replying to teor:

We might also want to check against the order in subsystem_list.c.

Yes, but have a look at check_and_setup() in subsysmgr.c -- that causes tor to exit with an assertion failure on startup if the subsystems are not sorted by priority.

comment:3 in reply to:  2 Changed 3 months ago by teor

Replying to nickm:

Replying to teor:

We might also want to check against the order in subsystem_list.c.

Yes, but have a look at check_and_setup() in subsysmgr.c -- that causes tor to exit with an assertion failure on startup if the subsystems are not sorted by priority.

So we don't need to check the order in subsystem_list.c, because tor does it for us. That's good.

comment:4 Changed 3 months ago by nickm

Type: defectenhancement

Mark a number of current 0.4.2.x "defects" as "enhancements."

comment:5 Changed 3 months ago by teor

Parent ID: #31615

comment:6 Changed 2 months ago by ahf

Owner: set to nickm
Status: newassigned

Distributing 0.4.2 tickets between network team members.

comment:7 Changed 2 months ago by nickm

Keywords: 042-should? added; 042-should removed
Reviewer: teor
Status: assignedneeds_review

I have started this, but I believe it is bigger than we should do in 0.4.2, and we should defer part of it till 0.4.3. Here is why:

  • Our subsystems do not currently to our directory structures. Notably, these subsystems are defined in with names that do not match their locations: btrack, network, ocirc_event, oconn_event, relay, threads, tortls, winprocess.
  • Our subsystem order does not currently match the topology very well at all: there are four discontinuities on the ordering. I think they are caused by: btrack, compress, winprocess, threads. We should re-order these, but doing so will require us to re-organize some of our code. That seems like a stability risk.

I have checked in a work-in-progress version of the code to do these checks as ticket31634. Do you think it's reasonable to defer to 0.4.3 based on the above reasoning? If not, please re-assign to me for 0.4.2, but this just got bigger than we anticipated.

comment:8 Changed 2 months ago by nickm

(Right now I am not asking for review on the code per se, since it is not ready. I'm asking for review on the decision to defer this to 0.4.3 when we willing to take more stability risks.)

comment:9 Changed 2 months ago by teor

Keywords: 043-should added; 042-should? removed
Milestone: Tor: 0.4.2.x-finalTor: 0.4.3.x-final
Status: needs_reviewneeds_revision

Yes, I think we should make big changes like this in 0.4.3.

And in 0.4.2, we should avoid:

  • changes to .may_include
  • changes to the module init order
Note: See TracTickets for help on using tickets.