Opened 9 years ago

Closed 6 years ago

#1992 closed defect (fixed)

relays resolve hostname every time they compute dir fetch schedule; but bridges resolve too seldom

Reported by: arma Owned by: arma
Priority: Medium Milestone: Tor: 0.2.4.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-relay
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

directory_fetches_from_authorities() calls router_pick_published_address() whenever server_mode(options) is true.

This happens during directory_get_from_dirserver(), directory_command_should_use_begindir(), dir_routerdesc_download_failed(), directory_info_has_arrived(), update_consensus_networkstatus_fetch_time(), launch_router_descriptor_downloads(), update_router_descriptor_downloads(), etc.

Sounds like we should refactor things so it writes its guessed address to a variable and just reads that variable, and then have a periodic event (scheduled when we want it, rather than scheduled by accident because we think about doing a directory fetch) that writes an address into the variable.

Child Tickets

Change History (19)

comment:1 Changed 9 years ago by arma

Right now we call check_descriptor_ipaddress_changed() every 15 minutes.

We also re-guess our address every time we get a directory answer (directory_info_has_arrived() calls directory_fetches_from_authorities() and update_router_descriptor_downloads() -- so that's twice in that function), and basically several times for each directory fetch as well.

In particular, update_router_descriptor_downloads() gets called every 60 seconds if we're lucky, and every 10 seconds while ! router_have_minimum_dir_info().

So I'd say if we crank CHECK_IPADDRESS_INTERVAL down to 60, and stop calling resolve_my_address() except from that function, we'll be a lot less spammy.

comment:2 Changed 9 years ago by arma

It's worth noting that bridge relays short-circuit in directory_fetches_from_authorities() so they don't call router_pick_published_address(). So they only notice IP changes after 15 minutes (or after a directory fetch, if sooner). That results in bug reports like #1913, that we could resolve by cranking down CHECK_IPADDRESS_INTERVAL. (I'm going to close that bug and point it to this one.)

comment:3 Changed 9 years ago by arma

Summary: relays resolve hostname every time they compute dir fetch schedulerelays resolve hostname every time they compute dir fetch schedule; but bridges resolve too seldom

comment:4 Changed 9 years ago by arma

Status: newneeds_review

Mildly related: check out bug1992_part1 in my arma. It does some refactoring that has been bugging me for a while.

It probably doesn't do the refactoring quite right. Nick, please feel free to take this as a "look how yucky Tor is" patch rather than a "here's how to make Tor not yucky" patch, and run with it.

comment:5 Changed 9 years ago by nickm

Looks ok except for some really trivial things:

  • the 'tmpbuf' thing doesn't necessarily get freed; I think there are some paths out of that function without calling tor_free(tmpbuf).
  • 'tmpbuf' is no longer a good name for it
  • The documentation for anything that takes a uint32_t as an IP address needs to be explicit about whether it wants one in host or network order. I personally prefer saying "format_addr_ipv4h" instead to be explicit.

I can clean up the above stuff and merge. Then we should do another grep for INET_NTOA_BUF_LEN to look for other places that might want converting.

Still trivial, not worth fixing here

  • I wish we had a naming convention for things that do the "return a pointer that the next call to this function will invalidate; VERY NONENTRANT" pattern.

comment:6 Changed 9 years ago by nickm

Owner: set to arma
Status: needs_reviewassigned

tweaked and merged. Not closing, since bug1992_part1 doesn't actually relate too much to bug #1992

comment:7 in reply to:  5 Changed 9 years ago by Sebastian

Replying to nickm:

  • The documentation for anything that takes a uint32_t as an IP address needs to be explicit about whether it wants one in host or network order. I personally prefer saying "format_addr_ipv4h" instead to be explicit.

Looks like you didn't address this comment?

comment:8 Changed 9 years ago by nickm

I think I made the documentation explicit. That should do for now.

comment:9 Changed 9 years ago by Sebastian

ok, now I agree :)

comment:10 Changed 7 years ago by nickm

Milestone: Tor: 0.2.3.x-finalTor: 0.2.4.x-final

comment:11 Changed 7 years ago by nickm

Keywords: tor-relay added

comment:12 Changed 7 years ago by nickm

Component: Tor RelayTor

comment:13 Changed 6 years ago by nickm

Milestone: Tor: 0.2.4.x-finalTor: 0.2.5.x-final

What part of this remains? Looks like we fixed some of it, or something related, in 2010?

comment:14 Changed 6 years ago by arma

I think both sides of the bug remain. When you run at debug-level logs, you can see way way way too many calls to resolve-my-address.

comment:15 Changed 6 years ago by arma

Status: assignednew

comment:16 Changed 6 years ago by arma

Priority: minornormal
Status: newneeds_review

See my bug1992 branch.

It should also resolve bug #2410.

I introduce a new METHOD for how we chose our IP address guess. But I don't think that method ever reaches a controller event, so I don't think there's any need to change control-spec.txt.

comment:17 Changed 6 years ago by nickm

First thoughts: (this should get another lookover before merging into 0.2.5. please also say if you think this should go into 0.2.4 instead.)

  • Wow, the resolve_my_address interface is getting ugly. Six arguments now? Where will it end? Is there any way to do this without turning it into even more of a swiss army function?

Other than that, seems plausible and good to me.

comment:18 Changed 6 years ago by arma

Milestone: Tor: 0.2.5.x-finalTor: 0.2.4.x-final

I just pushed another commit to my bug1992 to handle the caching notion differently. Perhaps you'll like it more.

I started off not feeling strongly about 0.2.4 vs 0.2.5, but now that I think about it, I think 0.2.4 might be smarter -- mostly since we've both looked at the patch recently, and if we wait til 0.2.5 that might turn bad.

I'm increasingly realizing that we have like 3 places that cache some sort of address guess, and 3 places that log when it changes. Some refactoring would be in order -- and that refactoring sounds like an 0.2.5 thing. Whereas #1992 and #2410 are actual bugs.

comment:19 Changed 6 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Looks okay -- merging. Please open the 0.2.5 refactoring ticket if you like and you still think it's a good idea.

Note: See TracTickets for help on using tickets.