Opened 4 years ago

Closed 3 years ago

#17590 closed enhancement (implemented)

Decouple connection_ap_handshake_attach_circuit from nearly everything.

Reported by: nickm Owned by:
Priority: Medium Milestone: Tor: 0.2.8.x-final
Component: Core Tor/Tor Version:
Severity: Minor Keywords: blob SponsorS refactor
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:


Long ago we used to call connection_ap_handshake_attach_circuit() only in a few places, since connection_ap_attach_pending() attaches all the pending connections, and does so regularly. But this turned out to have a performance problem: it would introduce a delay to launching or connecting a stream.

We couldn't just call connection_ap_attach_pending() every time we make a new connection, since it walks the whole connection list. So we started calling connection_ap_attach_pending all over, instead! But that's kind of ugly and messes up our callgraph.

But we have an opportunity to make Tor simpler!

  • We can make connection_ap_attach_pending() linear in the number of pending entry connections, rather than in the number of total connections.
  • If we do that, we can make it get called from whenever there is a pending entry connection from run_main_loop_once() or somewhere.
  • And if we do that, we can just put connections on a pending-list, rather than calling connection_ap_attach_pending() on them directly.

This will simplify tor, simplify our callgraph, and -- with the help of #17589 -- break the blob into multiple smaller strongly connected components.

Child Tickets

Change History (4)

comment:1 Changed 4 years ago by nickm

Status: newneeds_review

See decouple_conn_attach in my public repository.

With this change, the blob is now finally broken into a number of sub-blobs.

comment:2 Changed 4 years ago by mikeperry

I gave this a look, and modulo the XXX to free the pending list this seems OK to me. I have not spent a lot of time working with the entry connection attachment logic, though.

comment:3 Changed 4 years ago by nickm

I've cleaned up the logic a tiny bit to ensure it won't run away with the CPU, documented when the callbacks happened, fixed the XXX, and rebased. Now it's all in decouple_conn_attach_2.

(Note that the first patch in the branch has not changed, so there's no need to re-review that if you've already seen it.)

comment:4 Changed 3 years ago by nickm

Resolution: implemented
Status: needs_reviewclosed

Merged to master.

Note: See TracTickets for help on using tickets.