Defer reporting directory bootstrap progress

Existing cached directory information can cause misleadingly high
bootstrap percentages.  To improve user experience, defer reporting of
directory information progress until at least one connection has
succeeded to a relay or bridge.

Closes ticket 27169.

diff --git a/changes/ticket27169 b/changes/ticket27169
new file mode 100644 (file)
index 0000000..7854532
--- /dev/null
+++ b/changes/ticket27169
@@ -0,0 +1,6 @@
+  o Minor features (bootstrap):
+    - Improve user experience by deferring directory progress
+      reporting until after a connection to a relay or bridge has
+      succeeded.  This avoids reporting 80% progress based on cached
+      directory information when we can't even connect to a bridge or
+      relay.  Closes ticket 27169.

diff --git a/src/core/or/connection_or.c b/src/core/or/connection_or.c
index 08371d1ad7f36ceb9deb4c63faab0fd446de2e8b..e6b235401216719398477f1ba3e120a3d157434e 100644 (file)
--- a/src/core/or/connection_or.c
+++ b/src/core/or/connection_or.c
@@ -705,6 +705,7 @@ connection_or_finished_connecting(or_connection_t *or_conn)
   log_debug(LD_HANDSHAKE,"OR connect() to router at %s:%u finished.",
             conn->address,conn->port);
   control_event_bootstrap(BOOTSTRAP_STATUS_HANDSHAKE, 0);
+  control_event_boot_first_orconn();
 
   if (proxy_type != PROXY_NONE) {
     /* start proxy handshake */

diff --git a/src/core/or/relay.c b/src/core/or/relay.c
index 6e1adfaff3228201a0fbd392d99a13ad5a80b1e9..1d0be0c894bd6e2d29317e3cc1860f1189a19682 100644 (file)
--- a/src/core/or/relay.c
+++ b/src/core/or/relay.c
@@ -1389,8 +1389,8 @@ connection_edge_process_relay_cell_not_open(
         case DIR_PURPOSE_FETCH_SERVERDESC:
         case DIR_PURPOSE_FETCH_MICRODESC:
           if (TO_DIR_CONN(dirconn)->router_purpose == ROUTER_PURPOSE_GENERAL)
-            control_event_bootstrap(BOOTSTRAP_STATUS_LOADING_DESCRIPTORS,
-                                    count_loading_descriptors_progress());
+            control_event_boot_dir(BOOTSTRAP_STATUS_LOADING_DESCRIPTORS,
+                                   count_loading_descriptors_progress());
           break;
       }
     }

diff --git a/src/feature/control/control.c b/src/feature/control/control.c
index d68f1e3b07edcf50c47aaa9d613da9874846c612..2fac3bd6de95bc53ab01c9de93e1e5fd84eb2f48 100644 (file)
--- a/src/feature/control/control.c
+++ b/src/feature/control/control.c
@@ -7178,6 +7178,45 @@ control_event_bootstrap(bootstrap_status_t status, int progress)
   }
 }
 
+/** Flag whether we've opened an OR_CONN yet  */
+static int bootstrap_first_orconn = 0;
+
+/** Like bootstrap_phase, but for (possibly deferred) directory progress */
+static int bootstrap_dir_phase = BOOTSTRAP_STATUS_UNDEF;
+
+/** Like bootstrap_problems, but for (possibly deferred) directory progress  */
+static int bootstrap_dir_progress = BOOTSTRAP_STATUS_UNDEF;
+
+/** Defer directory info bootstrap events until we have successfully
+ * completed our first connection to a router.  */
+void
+control_event_boot_dir(bootstrap_status_t status, int progress)
+{
+  if (status > bootstrap_dir_progress) {
+    bootstrap_dir_progress = status;
+    bootstrap_dir_phase = status;
+  }
+  if (progress && progress >= bootstrap_dir_progress) {
+    bootstrap_dir_progress = progress;
+  }
+
+  /* Don't report unless we have successfully opened at least one OR_CONN */
+  if (!bootstrap_first_orconn)
+    return;
+
+  control_event_bootstrap(status, progress);
+}
+
+/** Set a flag to allow reporting of directory bootstrap progress.
+ * (Code that reports completion of an OR_CONN calls this.)  Also,
+ * report directory progress so far. */
+void
+control_event_boot_first_orconn(void)
+{
+  bootstrap_first_orconn = 1;
+  control_event_bootstrap(bootstrap_dir_phase, bootstrap_dir_progress);
+}
+
 /** Called when Tor has failed to make bootstrapping progress in a way
  * that indicates a problem. <b>warn</b> gives a human-readable hint
  * as to why, and <b>reason</b> provides a controller-facing short

diff --git a/src/feature/control/control.h b/src/feature/control/control.h
index 7f57228e5c4371324c8a48a4c6b899db4c73a751..3445eb0a9d663ca5505dec4abbc55aab68450aff 100644 (file)
--- a/src/feature/control/control.h
+++ b/src/feature/control/control.h
@@ -195,6 +195,8 @@ void control_event_bootstrap(bootstrap_status_t status, int progress);
 MOCK_DECL(void, control_event_bootstrap_prob_or,(const char *warn,
                                                  int reason,
                                                  or_connection_t *or_conn));
+void control_event_boot_dir(bootstrap_status_t status, int progress);
+void control_event_boot_first_orconn(void);
 void control_event_bootstrap_problem(const char *warn, const char *reason,
                                      const connection_t *conn, int dowarn);
 

diff --git a/src/feature/dircache/directory.c b/src/feature/dircache/directory.c
index de0bcdbfa79f0d575b3b63b88562a4177f20b1db..b94c5317af38d5379f63c06a28cafab491a07648 100644 (file)
--- a/src/feature/dircache/directory.c
+++ b/src/feature/dircache/directory.c
@@ -2226,8 +2226,8 @@ load_downloaded_routers(const char *body, smartlist_t *which,
   added = router_load_routers_from_string(body, NULL, SAVED_NOWHERE, which,
                                   descriptor_digests, buf);
   if (added && general)
-    control_event_bootstrap(BOOTSTRAP_STATUS_LOADING_DESCRIPTORS,
-                            count_loading_descriptors_progress());
+    control_event_boot_dir(BOOTSTRAP_STATUS_LOADING_DESCRIPTORS,
+                           count_loading_descriptors_progress());
   return added;
 }
 
@@ -2949,8 +2949,8 @@ handle_response_fetch_microdesc(dir_connection_t *conn,
       dir_microdesc_download_failed(which, status_code, conn->identity_digest);
     }
     if (mds && smartlist_len(mds)) {
-      control_event_bootstrap(BOOTSTRAP_STATUS_LOADING_DESCRIPTORS,
-                              count_loading_descriptors_progress());
+      control_event_boot_dir(BOOTSTRAP_STATUS_LOADING_DESCRIPTORS,
+                             count_loading_descriptors_progress());
       directory_info_has_arrived(now, 0, 1);
     }
     SMARTLIST_FOREACH(which, char *, cp, tor_free(cp));

diff --git a/src/feature/nodelist/nodelist.c b/src/feature/nodelist/nodelist.c
index 6dd501ea346ed6a5cad88a463fcc25b77031b17c..50dc8f7d3cc2d6088d6545f0b04ffe844add266e 100644 (file)
--- a/src/feature/nodelist/nodelist.c
+++ b/src/feature/nodelist/nodelist.c
@@ -2528,7 +2528,7 @@ update_router_have_minimum_dir_info(void)
                    (int)(paths*100), status);
       tor_free(status);
       res = 0;
-      control_event_bootstrap(BOOTSTRAP_STATUS_REQUESTING_DESCRIPTORS, 0);
+      control_event_boot_dir(BOOTSTRAP_STATUS_REQUESTING_DESCRIPTORS, 0);
       goto done;
     }
 
@@ -2553,7 +2553,7 @@ update_router_have_minimum_dir_info(void)
   /* If paths have just become available in this update. */
   if (res && !have_min_dir_info) {
     control_event_client_status(LOG_NOTICE, "ENOUGH_DIR_INFO");
-    control_event_bootstrap(BOOTSTRAP_STATUS_CONN_OR, 0);
+    control_event_boot_dir(BOOTSTRAP_STATUS_CONN_OR, 0);
     log_info(LD_DIR,
              "We now have enough directory information to build circuits.");
   }

diff --git a/src/test/test_controller_events.c b/src/test/test_controller_events.c
index e935b7042840a6ef8b0481f267b0418bf1843e22..4c404876b031f86285a31dd7f03ba15afc4bf6c5 100644 (file)
--- a/src/test/test_controller_events.c
+++ b/src/test/test_controller_events.c
@@ -322,6 +322,72 @@ test_cntev_event_mask(void *arg)
   ;
 }
 
+static char *saved_event_str = NULL;
+
+static void
+mock_queue_control_event_string(uint16_t event, char *msg)
+{
+  (void)event;
+
+  tor_free(saved_event_str);
+  saved_event_str = msg;
+}
+
+/* Helper macro for checking bootstrap control event strings */
+#define assert_bootmsg(s)                                               \
+  tt_ptr_op(strstr(saved_event_str, "650 STATUS_CLIENT NOTICE "         \
+                   "BOOTSTRAP PROGRESS=" s), OP_EQ, saved_event_str)
+
+/* Test deferral of directory bootstrap messages (requesting_descriptors) */
+static void
+test_cntev_dirboot_defer_desc(void *arg)
+{
+  (void)arg;
+
+  MOCK(queue_control_event_string, mock_queue_control_event_string);
+  control_testing_set_global_event_mask(EVENT_MASK_(EVENT_STATUS_CLIENT));
+  control_event_bootstrap(BOOTSTRAP_STATUS_STARTING, 0);
+  assert_bootmsg("0 TAG=starting");
+  /* This event should get deferred */
+  control_event_boot_dir(BOOTSTRAP_STATUS_REQUESTING_DESCRIPTORS, 0);
+  assert_bootmsg("0 TAG=starting");
+  control_event_bootstrap(BOOTSTRAP_STATUS_CONN_DIR, 0);
+  assert_bootmsg("5 TAG=conn_dir");
+  control_event_bootstrap(BOOTSTRAP_STATUS_HANDSHAKE, 0);
+  assert_bootmsg("10 TAG=handshake_dir");
+  /* The deferred event should appear */
+  control_event_boot_first_orconn();
+  assert_bootmsg("45 TAG=requesting_descriptors");
+ done:
+  tor_free(saved_event_str);
+  UNMOCK(queue_control_event_string);
+}
+
+/* Test deferral of directory bootstrap messages (conn_or) */
+static void
+test_cntev_dirboot_defer_orconn(void *arg)
+{
+  (void)arg;
+
+  MOCK(queue_control_event_string, mock_queue_control_event_string);
+  control_testing_set_global_event_mask(EVENT_MASK_(EVENT_STATUS_CLIENT));
+  control_event_bootstrap(BOOTSTRAP_STATUS_STARTING, 0);
+  assert_bootmsg("0 TAG=starting");
+  /* This event should get deferred */
+  control_event_boot_dir(BOOTSTRAP_STATUS_CONN_OR, 0);
+  assert_bootmsg("0 TAG=starting");
+  control_event_bootstrap(BOOTSTRAP_STATUS_CONN_DIR, 0);
+  assert_bootmsg("5 TAG=conn_dir");
+  control_event_bootstrap(BOOTSTRAP_STATUS_HANDSHAKE, 0);
+  assert_bootmsg("10 TAG=handshake_dir");
+  /* The deferred event should appear */
+  control_event_boot_first_orconn();
+  assert_bootmsg("80 TAG=conn_or");
+ done:
+  tor_free(saved_event_str);
+  UNMOCK(queue_control_event_string);
+}
+
 #define TEST(name, flags)                                               \
   { #name, test_cntev_ ## name, flags, 0, NULL }
 
@@ -330,5 +396,7 @@ struct testcase_t controller_event_tests[] = {
   TEST(append_cell_stats, TT_FORK),
   TEST(format_cell_stats, TT_FORK),
   TEST(event_mask, TT_FORK),
+  TEST(dirboot_defer_desc, TT_FORK),
+  TEST(dirboot_defer_orconn, TT_FORK),
   END_OF_TESTCASES
 };