Prevent a "try forever to download an unobtainable descriptor" bug. Still remaining: the "give up forever" bug.

svn:r5090

5 files changed, 32 insertions, 17 deletions

diff --git a/doc/TODO b/doc/TODO
index 965f08406c..380c2dc3ae 100644
--- a/doc/TODO
+++ b/doc/TODO
@@ -133,7 +133,7 @@ R     - check reachability as soon as you hear about a new server
         X By 'if-newer-than' (Does the spec require this??)
         o Support compression.
 N     - Alice acts on network-status objects
-        . Alice downloads descriptors as needed.
+        o Alice downloads descriptors as needed.
           o Figure out what's needed
           o Store it
             o Implement store
@@ -146,7 +146,8 @@ N     - Alice acts on network-status objects
             o Call directory_has_arrived as needed; rename it.
             o Set has_fetched_directory properly.
           o Retry descriptors on failure
-          - Give up after a while.
+          o Give up after a while.
+          - But try again after a long while (???)
         - Check software versions according to some sane plan.
         o Alice sets descriptor status from network-status
           o Implement
diff --git a/src/or/directory.c b/src/or/directory.c
index 031828be8d..3bf2cf857d 100644
--- a/src/or/directory.c
+++ b/src/or/directory.c
@@ -283,7 +283,7 @@ connection_dir_request_failed(connection_t *conn)
     log_fn(LOG_INFO, "Giving up on directory server at '%s'; retrying",
            conn->address);
     connection_dir_download_networkstatus_failed(conn);
-  } else if (conn->purpose == DIR_PURPOSE_FETCH_NETWORKSTATUS) {
+  } else if (conn->purpose == DIR_PURPOSE_FETCH_SERVERDESC) {
     log_fn(LOG_INFO, "Giving up on directory server at '%s'; retrying",
            conn->address);
     connection_dir_download_routerdesc_failed(conn);
@@ -320,7 +320,8 @@ connection_dir_download_networkstatus_failed(connection_t *conn)
 static void
 connection_dir_download_routerdesc_failed(connection_t *conn)
 {
-  /* try again. */
+  /* Try again. */
+  /*XXXX011 plays poorly with multiple conns. */
   update_router_descriptor_downloads(time(NULL));
 }
 
@@ -1547,7 +1548,13 @@ dir_networkstatus_download_failed(smartlist_t *failed)
 static void
 dir_routerdesc_download_failed(smartlist_t *failed)
 {
-  /* XXXX writeme!  Give up after a while! */
+  SMARTLIST_FOREACH(failed, const char *, cp,
+  {
+    routerstatus_t *rs = router_get_combined_status_by_digest(cp);
+    if (!rs || rs->n_download_failures >= MAX_ROUTERDESC_DOWNLOAD_FAILURES)
+      continue;
+    ++rs->n_download_failures;
+  });
 }
 
 /* DOCDOC */
diff --git a/src/or/or.h b/src/or/or.h
index 14004a9020..a710df06ae 100644
--- a/src/or/or.h
+++ b/src/or/or.h
@@ -805,6 +805,9 @@ typedef struct routerstatus_t {
                                 * recent descriptor. */
 } routerstatus_t;
 
+/*XXXX001 make this configurable! */
+#define MAX_ROUTERDESC_DOWNLOAD_FAILURES 16
+
 /** Contents of a (v2 or later) network status object. */
 typedef struct networkstatus_t {
   /** When did we receive the network-status document? */
@@ -2107,11 +2110,13 @@ void add_trusted_dir_server(const char *addr, uint16_t port,
                             const char *digest, int supports_v1);
 void clear_trusted_dir_servers(void);
 networkstatus_t *networkstatus_get_by_digest(const char *digest);
+routerstatus_t *router_get_combined_status_by_digest(const char *digest);
 void update_networkstatus_cache_downloads(time_t now);
 void update_networkstatus_client_downloads(time_t now);
 void update_router_descriptor_downloads(time_t now);
 void routers_update_all_from_networkstatus(void);
-void routers_update_status_from_networkstatus(smartlist_t *routers);
+void routers_update_status_from_networkstatus(smartlist_t *routers,
+                                              int reset_failures);
 smartlist_t *router_list_superseded(void);
 int router_have_minimum_dir_info(void);
 void networkstatus_list_update_recent(time_t now);
diff --git a/src/or/routerlist.c b/src/or/routerlist.c
index 9f3068fa34..e4d4f2d92f 100644
--- a/src/or/routerlist.c
+++ b/src/or/routerlist.c
@@ -29,7 +29,6 @@ static trusted_dir_server_t *router_pick_trusteddirserver_impl(
 static void mark_all_trusteddirservers_up(void);
 static int router_nickname_is_in_list(routerinfo_t *router, const char *list);
 static int router_nickname_matches(routerinfo_t *router, const char *nickname);
-static routerstatus_t *router_get_combined_status_by_digest(const char *d);
 static void routerstatus_list_update_from_networkstatus(time_t now);
 
 /****************************************************************************/
@@ -1292,7 +1291,7 @@ router_load_routers_from_string(const char *s, int from_cache,
 
   router_parse_list_from_string(&s, routers);
 
-  routers_update_status_from_networkstatus(routers);
+  routers_update_status_from_networkstatus(routers, !from_cache);
 
   SMARTLIST_FOREACH(routers, routerinfo_t *, ri,
   {
@@ -1554,7 +1553,8 @@ networkstatus_find_entry(networkstatus_t *ns, const char *digest)
                            _compare_digest_to_routerstatus_entry);
 }
 
-static routerstatus_t *
+/*DOCDOC*/
+routerstatus_t *
 router_get_combined_status_by_digest(const char *digest)
 {
   if (!routerstatus_list)
@@ -1563,7 +1563,6 @@ router_get_combined_status_by_digest(const char *digest)
                            _compare_digest_to_routerstatus_entry);
 }
 
-
 /* XXXX These should be configurable, perhaps? NM */
 #define AUTHORITY_NS_CACHE_INTERVAL 10*60
 #define NONAUTHORITY_NS_CACHE_INTERVAL 15*60
@@ -1995,7 +1994,7 @@ routers_update_all_from_networkstatus(void)
   if (networkstatus_list_has_changed)
     routerstatus_list_update_from_networkstatus(now);
 
-  routers_update_status_from_networkstatus(routerlist->routers);
+  routers_update_status_from_networkstatus(routerlist->routers, 0);
 
   me = router_get_my_routerinfo();
   if (me) {
@@ -2003,7 +2002,7 @@ routers_update_all_from_networkstatus(void)
      * dirservers list us as named, valid, etc. */
     smartlist_t *lst = smartlist_create();
     smartlist_add(lst, me);
-    routers_update_status_from_networkstatus(lst);
+    routers_update_status_from_networkstatus(lst, 1);
     if (me->is_verified == 0) {
       log_fn(LOG_WARN, "Many directory servers list us as unverified. Please consider sending your identity fingerprint to the tor-ops.");
       have_warned_about_unverified_status = 1;
@@ -2179,7 +2178,7 @@ routerstatus_list_update_from_networkstatus(time_t now)
  * is_named, is_verified, and is_running fields according to our current
  * networkstatus_t documents. */
 void
-routers_update_status_from_networkstatus(smartlist_t *routers)
+routers_update_status_from_networkstatus(smartlist_t *routers, int reset_failures)
 {
   trusted_dir_server_t *ds;
   routerstatus_t *rs;
@@ -2198,6 +2197,9 @@ routers_update_status_from_networkstatus(smartlist_t *routers)
     if (!rs)
       continue;
 
+    if (reset_failures)
+      rs->n_download_failures = 0;
+
     if (!namingdir)
       router->is_named = rs->is_named;
 
@@ -2250,8 +2252,10 @@ router_list_downloadable(void)
         // log_fn(LOG_NOTICE, "No status for %s", fp);
         continue;
       }
-      if (!memcmp(ri->signed_descriptor_digest,rs->descriptor_digest,DIGEST_LEN)
-          || rs->published_on <= ri->published_on) {
+      /*XXXX001 reset max_routerdesc_download_failures somewhere! */
+      if (rs->n_download_failures >= MAX_ROUTERDESC_DOWNLOAD_FAILURES ||
+          !memcmp(ri->signed_descriptor_digest,rs->descriptor_digest,DIGEST_LEN)||
+          rs->published_on <= ri->published_on) {
         /* Same digest, or earlier. No need to download it. */
         // log_fn(LOG_NOTICE, "Up-to-date status for %s", fp);
         strmap_remove(status_map, fp);
diff --git a/src/or/routerparse.c b/src/or/routerparse.c
index b3f780c7ba..58342d3df2 100644
--- a/src/or/routerparse.c
+++ b/src/or/routerparse.c
@@ -692,8 +692,6 @@ router_parse_list_from_string(const char **s, smartlist_t *dest)
     smartlist_add(routers, router);
   }
 
-  routers_update_status_from_networkstatus(routers);
-
   smartlist_add_all(dest, routers);
 
   return 0;