Opened 6 weeks ago

Last modified 5 weeks ago

#31615 merge_ready defect

Reorder the early subsystems based on their dependencies

Reported by: teor Owned by: teor
Priority: Medium Milestone: Tor: 0.4.1.x-final
Component: Core Tor/Tor Version: Tor: 0.4.0.1-alpha
Severity: Normal Keywords: diagnostics, consider-backport-after-042-stable, consider-backport-if-needed, 042-should, 040-backport-maybe, 041-backport-maybe, BugSmashFund, dgoulet-merge
Cc: Actual Points: 0.3
Parent ID: Points: 0.2
Reviewer: nickm Sponsor:

Description (last modified by teor)

Some of our subsystem dependencies are out of sync with their module dependencies.

So if these early subsystems log, we might not be able to see the errors. (Or the process might crash.)

Child Tickets

Change History (16)

comment:1 Changed 6 weeks ago by nickm

Possibly helpful: you can run ./scripts/maint/practracker/includes.py --toposort to get the modules covered by .may_include, topologically sorted by which other modules they depend on.

comment:2 in reply to:  1 Changed 6 weeks ago by teor

Replying to nickm:

Possibly helpful: you can run ./scripts/maint/practracker/includes.py --toposort to get the modules covered by .may_include, topologically sorted by which other modules they depend on.

Hmm, so there's another check that practracker could do: make sure the subsystem init order corresponds to the topological sort order,

comment:3 Changed 6 weeks ago by nickm

Yeah -- I think in this case what we want to do might be to make a debugging command for tor to dump the subsystem init order, so we don't need to parse it out of the source.

comment:4 Changed 6 weeks ago by teor

Yeah and we can't parse it out of the logs, because the buggy module order was in modules that were initialised before the logs.

I opened #31634 for that change. It's a big one, so I'm not going to do anything about it now.

comment:5 Changed 6 weeks ago by teor

Actual Points: 0.2
Cc: gaba removed
Description: modified (diff)
Keywords: BugSmash added; android macos 035-backport removed
Points: 0.2
Status: newneeds_review
Version: Tor: 0.4.0.1-alpha

See my pull request:

The merge forward was clean.

Here are the test branches for merging forwards:

comment:6 Changed 5 weeks ago by teor

Keywords: 040-backport-maybe 041-backport-maybe added; 040-backport 041-backport removed

comment:7 Changed 5 weeks ago by teor

Keywords: consider-backport-after-042-stable consider-backport-if-needed added

I don't think we should backport this fix, unless the bug is actually causing issues in older versions.

comment:8 Changed 5 weeks ago by teor

Owner: set to teor
Status: needs_reviewassigned

comment:9 Changed 5 weeks ago by teor

Status: assignedneeds_review

comment:10 Changed 5 weeks ago by teor

Keywords: BugSmashFund added; BugSmash removed

Fix bug smash fund spelling

comment:11 Changed 5 weeks ago by dgoulet

Reviewer: nickm

comment:12 Changed 5 weeks ago by nickm

Status: needs_reviewneeds_revision

These look good, except that the levels we have here are now out-of-sync with the comments in tor_subsystems.c. Please feel free to either edit the ones in tor_subsystems.c, or remove them entirely, and put this back into merge_ready once you've done so.

comment:13 Changed 5 weeks ago by teor

Parent ID: #31594

This ticket is independent of its parent.

comment:14 Changed 5 weeks ago by teor

Actual Points: 0.20.3
Status: needs_revisionmerge_ready

I removed the comments, the merge forward had trivial conflicts:

CI is still running on 0.4.1 and master, but I only changed comments, so I think it will pass.

comment:15 Changed 5 weeks ago by nickm

Keywords: dgoulet-merge added

(CI has passed)

comment:16 Changed 5 weeks ago by dgoulet

Milestone: Tor: 0.4.2.x-finalTor: 0.4.1.x-final

Merged to master. Moving to 041 milestone for backport.

Note: See TracTickets for help on using tickets.