Opened 6 years ago

Closed 5 years ago

#12205 closed defect (fixed)

Document magic numbers at entry_is_time_to_retry()

Reported by: asn Owned by: rl1987
Priority: Very Low Milestone: Tor: 0.2.6.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-guard
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

 if (diff < 6*60*60)
    return now > (e->last_attempted + 60*60);
  else if (diff < 3*24*60*60)
    return now > (e->last_attempted + 4*60*60);
  else if (diff < 7*24*60*60)
    return now > (e->last_attempted + 18*60*60);
  else
    return now > (e->last_attempted + 36*60*60);
}

This is our entry guard retry logic. We should probably document these numbers, so that they look less magic.

Child Tickets

Change History (13)

comment:1 Changed 5 years ago by asn

Milestone: Tor: 0.2.6.x-final

comment:2 Changed 5 years ago by asn

Priority: minortrivial

FWIW, this is actually documented in path-spec.txt:

  If Tor fails to connect to an otherwise usable guard, it retries
  periodically: every hour for six hours, every 4 hours for 3 days, every
  18 hours for a week, and every 36 hours thereafter.  Additionally, Tor
  retries unreachable guards the first time it adds a new guard to the list,
  since it is possible that the old guards were only marked as unreachable
  because the network was unreachable or down.

Still, it might be worth mentioning this in the code.

comment:3 Changed 5 years ago by asn

Milestone: Tor: 0.2.6.x-finalTor: 0.2.???

Downgrading to the ?? milestone because of low importance.

comment:4 Changed 5 years ago by rl1987

Owner: set to rl1987
Status: newassigned

comment:5 Changed 5 years ago by rl1987

Status: assignedneeds_review

Proposed changes in following branch: 

https://github.com/rl1987/tor/tree/bug12205

comment:6 Changed 5 years ago by asn

Milestone: Tor: 0.2.???Tor: 0.2.6.x-final

Nice!

Haven't reviewed yet. Will do soon.

I guess we can upgrade this to 0.2.6 now that it has a patch.

comment:7 Changed 5 years ago by asn

patch looks sane to me.

Last edited 5 years ago by asn (previous) (diff)

comment:8 Changed 5 years ago by nickm

I don't actually like this kind of stuff:

 #define SECONDS_IN_AN_HOUR (60*60)

To me, it seems not much better than the classic:

 #define FOUR (4)

The reason for using symbolic constants is to make intent clear. When clarifying entry_is_time_to_retry, the question is not "what does 60*60 mean" -- it's obviously an hour -- but rather "why are we looking at 60*60 here?"

Looking at the intent for this function, it is apparently to schedule attempts frequently at first, and then more slowly later on. So maybe something like:

    if (diff < ENTRY_RETRY_PERIOD_1)
      return now > (e->last_attempted + ENTRY_RETRY_INTERVAL_1)
    else if (diff < ENTRY_RETRY_PERIOD_2)
       ...

Or even make it table-driven, as in:

    struct entry_retry_periods {
      time_t period_length;  time_t interval_during_period;
    } periods[] = {
     {    6*60*60,    60*60 }, /* For the first 6 hours, retry hourly. */
     { 3*24*60*60,  4*60*60 }, /* Then try every 4 hours until the 3-day mark. */
     { 7*24*60*60, 18*60*60 }, /* Then every 18 hours until the 7-day mark. */
     {  TIME_MAX,  36*60*60 }, /* Then every 36 hours thereafter. */
    }
    int i;
    for (i = 0; ; ++i) {
      if (diff <= periods[i].period_length)
        return now > (e->last_attempted + periods[i].interval_during_period) ;
    }

I dunno; what do you think?

comment:9 Changed 5 years ago by nickm

Status: needs_reviewneeds_revision

comment:10 Changed 5 years ago by rl1987

Status: needs_revisionneeds_review

I refactored the aforementioned function using table approach:

https://github.com/rl1987/tor/commits/bug12205_take2

This seems to be begging for unit test though. Or is this function tested implicitly by test(s) of other function(s)?

comment:11 Changed 5 years ago by nickm

quick review:

1) Why did this go away?

-  if (e->last_attempted < e->unreachable_since)
-    return 1;

2) There's no advantage to using 'short' here.

3) Yeah, it should probably have a quick test.

comment:12 Changed 5 years ago by rl1987

Fixed these issues and added unit test. Please review changes in https://github.com/rl1987/tor/commits/bug12205_take2 .

comment:13 Changed 5 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Thanks! Squashed, tweaked a bit more, and merged.

Note: See TracTickets for help on using tickets.