ns: Add a before and after consensus has changed notification

In 0.3.2.1-alpha, we've added notify_networkstatus_changed() in order to have
a way to notify other subsystems that the consensus just changed. The old and
new consensus are passed to it.

Before this patch, this was done _before_ the new consensus was set globally
(thus NOT accessible by getting the latest consensus). The scheduler
notification was assuming that it was set and select_scheduler() is looking at
the latest consensus to get the parameters it might needs. This was very wrong
because at that point it is still the old consensus set globally.

This commit changes the notify_networkstatus_changed() to be the "before"
function and adds an "after" notification from which the scheduler subsystem
is notified.

Fixes #24975

diff --git a/changes/bug24975 b/changes/bug24975
new file mode 100644 (file)
index 0000000..32a5dfc
--- /dev/null
+++ b/changes/bug24975
@@ -0,0 +1,6 @@
+  o Major bugfixes (scheduler, consensus):
+    - A logic in the code was preventing the scheduler subystem to properly
+      make a decision based on the latest consensus when it arrives. This lead
+      to the scheduler failing to notice any consensus parameters that might
+      have changed between consensuses. Fixes bug 24975; bugfix on
+      0.3.2.1-alpha.

diff --git a/src/or/networkstatus.c b/src/or/networkstatus.c
index e0a3e4cdc627d45efaf471a61d61aea68cf1b588..9d9a2b8ad1ccfd96481ab059435035a6362046be 100644 (file)
--- a/src/or/networkstatus.c
+++ b/src/or/networkstatus.c
@@ -1564,13 +1564,20 @@ notify_control_networkstatus_changed(const networkstatus_t *old_c,
   smartlist_free(changed);
 }
 
-/* Called when the consensus has changed from old_c to new_c. */
+/* Called before the consensus changes from old_c to new_c. */
 static void
-notify_networkstatus_changed(const networkstatus_t *old_c,
-                             const networkstatus_t *new_c)
+notify_before_networkstatus_changes(const networkstatus_t *old_c,
+                                    const networkstatus_t *new_c)
 {
   notify_control_networkstatus_changed(old_c, new_c);
-  scheduler_notify_networkstatus_changed(old_c, new_c);
+}
+
+/* Called after a new consensus has been put in the global state. It is safe
+ * to use the consensus getters in this function. */
+static void
+notify_after_networkstatus_changes(void)
+{
+  scheduler_notify_networkstatus_changed();
 }
 
 /** Copy all the ancillary information (like router download status and so on)
@@ -1897,8 +1904,11 @@ networkstatus_set_current_consensus(const char *consensus,
 
   const int is_usable_flavor = flav == usable_consensus_flavor();
 
+  /* Before we switch to the new consensus, notify that we are about to change
+   * it using the old consensus and the new one. */
   if (is_usable_flavor) {
-    notify_networkstatus_changed(networkstatus_get_latest_consensus(), c);
+    notify_before_networkstatus_changes(networkstatus_get_latest_consensus(),
+                                        c);
   }
   if (flav == FLAV_NS) {
     if (current_ns_consensus) {
@@ -1941,6 +1951,10 @@ networkstatus_set_current_consensus(const char *consensus,
   }
 
   if (is_usable_flavor) {
+    /* Notify that we just changed the consensus so the current global value
+     * can be looked at. */
+    notify_after_networkstatus_changes();
+
     /* The "current" consensus has just been set and it is a usable flavor so
      * the first thing we need to do is recalculate the voting schedule static
      * object so we can use the timings in there needed by some subsystems

diff --git a/src/or/scheduler.c b/src/or/scheduler.c
index cbf51447bfe11986db19ff010713ed7fc31fc2f5..b68e48da076514fd8d2a2461bed57ce54d4d025b 100644 (file)
--- a/src/or/scheduler.c
+++ b/src/or/scheduler.c
@@ -435,15 +435,14 @@ scheduler_conf_changed(void)
  * Whenever we get a new consensus, this function is called.
  */
 void
-scheduler_notify_networkstatus_changed(const networkstatus_t *old_c,
-                                       const networkstatus_t *new_c)
+scheduler_notify_networkstatus_changed(void)
 {
   /* Maybe the consensus param made us change the scheduler. */
   set_scheduler();
 
   /* Then tell the (possibly new) scheduler that we have a new consensus */
   if (the_scheduler->on_new_consensus) {
-    the_scheduler->on_new_consensus(old_c, new_c);
+    the_scheduler->on_new_consensus();
   }
 }
 

diff --git a/src/or/scheduler.h b/src/or/scheduler.h
index 47c98f096ab4074f3da0420fc3c4306f283d73c8..559f1c8afcd7adee3d53ae25e3c65db4a061af91 100644 (file)
--- a/src/or/scheduler.h
+++ b/src/or/scheduler.h
@@ -80,8 +80,7 @@ typedef struct scheduler_s {
    * (which might be new) will call this so it has the chance to react to the
    * new consensus too. If there's a consensus parameter that your scheduler
    * wants to keep an eye on, this is where you should check for it.  */
-  void (*on_new_consensus)(const networkstatus_t *old_c,
-                           const networkstatus_t *new_c);
+  void (*on_new_consensus)(void);
 
   /* (Optional) To be called when a channel is being freed. Sometimes channels
    * go away (for example: the relay on the other end is shutting down). If
@@ -119,8 +118,7 @@ typedef struct scheduler_s {
 void scheduler_init(void);
 void scheduler_free_all(void);
 void scheduler_conf_changed(void);
-void scheduler_notify_networkstatus_changed(const networkstatus_t *old_c,
-                                            const networkstatus_t *new_c);
+void scheduler_notify_networkstatus_changed(void);
 MOCK_DECL(void, scheduler_release_channel, (channel_t *chan));
 
 /*
@@ -197,7 +195,7 @@ int scheduler_can_use_kist(void);
 void scheduler_kist_set_full_mode(void);
 void scheduler_kist_set_lite_mode(void);
 scheduler_t *get_kist_scheduler(void);
-int kist_scheduler_run_interval(const networkstatus_t *ns);
+int kist_scheduler_run_interval(void);
 
 #ifdef TOR_UNIT_TESTS
 extern int32_t sched_run_interval;

diff --git a/src/or/scheduler_kist.c b/src/or/scheduler_kist.c
index f50f3aa9e91d7b2ca68754b29d3b5ae4aaafa0b0..877e561e01b08e3bb96b2f38dd3728d965f09b33 100644 (file)
--- a/src/or/scheduler_kist.c
+++ b/src/or/scheduler_kist.c
@@ -362,10 +362,10 @@ outbuf_table_remove(outbuf_table_t *table, channel_t *chan)
 
 /* Set the scheduler running interval. */
 static void
-set_scheduler_run_interval(const networkstatus_t *ns)
+set_scheduler_run_interval(void)
 {
   int old_sched_run_interval = sched_run_interval;
-  sched_run_interval = kist_scheduler_run_interval(ns);
+  sched_run_interval = kist_scheduler_run_interval();
   if (old_sched_run_interval != sched_run_interval) {
     log_info(LD_SCHED, "Scheduler KIST changing its running interval "
                        "from %" PRId32 " to %" PRId32,
@@ -481,13 +481,9 @@ kist_on_channel_free(const channel_t *chan)
 
 /* Function of the scheduler interface: on_new_consensus() */
 static void
-kist_scheduler_on_new_consensus(const networkstatus_t *old_c,
-                                const networkstatus_t *new_c)
+kist_scheduler_on_new_consensus(void)
 {
-  (void) old_c;
-  (void) new_c;
-
-  set_scheduler_run_interval(new_c);
+  set_scheduler_run_interval();
 }
 
 /* Function of the scheduler interface: on_new_options() */
@@ -497,7 +493,7 @@ kist_scheduler_on_new_options(void)
   sock_buf_size_factor = get_options()->KISTSockBufSizeFactor;
 
   /* Calls kist_scheduler_run_interval which calls get_options(). */
-  set_scheduler_run_interval(NULL);
+  set_scheduler_run_interval();
 }
 
 /* Function of the scheduler interface: init() */
@@ -770,7 +766,7 @@ get_kist_scheduler(void)
  *   - If consensus doesn't say anything, return 10 milliseconds, default.
  */
 int
-kist_scheduler_run_interval(const networkstatus_t *ns)
+kist_scheduler_run_interval(void)
 {
   int run_interval = get_options()->KISTSchedRunInterval;
 
@@ -784,7 +780,7 @@ kist_scheduler_run_interval(const networkstatus_t *ns)
 
   /* Will either be the consensus value or the default. Note that 0 can be
    * returned which means the consensus wants us to NOT use KIST. */
-  return networkstatus_get_param(ns, "KISTSchedRunInterval",
+  return networkstatus_get_param(NULL, "KISTSchedRunInterval",
                                  KIST_SCHED_RUN_INTERVAL_DEFAULT,
                                  KIST_SCHED_RUN_INTERVAL_MIN,
                                  KIST_SCHED_RUN_INTERVAL_MAX);
@@ -823,7 +819,7 @@ scheduler_can_use_kist(void)
 
   /* We do have the support, time to check if we can get the interval that the
    * consensus can be disabling. */
-  int run_interval = kist_scheduler_run_interval(NULL);
+  int run_interval = kist_scheduler_run_interval();
   log_debug(LD_SCHED, "Determined KIST sched_run_interval should be "
                       "%" PRId32 ". Can%s use KIST.",
            run_interval, (run_interval > 0 ? "" : " not"));

diff --git a/src/test/test_scheduler.c b/src/test/test_scheduler.c
index 18f98951464bb62738c18743bb20bdc89be0e8fc..1f014c4f6a126d7e7473a8403f952c29381f2ad7 100644 (file)
--- a/src/test/test_scheduler.c
+++ b/src/test/test_scheduler.c
@@ -959,7 +959,7 @@ test_scheduler_can_use_kist(void *arg)
   clear_options();
   mocked_options.KISTSchedRunInterval = 1234;
   res_should = scheduler_can_use_kist();
-  res_freq = kist_scheduler_run_interval(NULL);
+  res_freq = kist_scheduler_run_interval();
 #ifdef HAVE_KIST_SUPPORT
   tt_int_op(res_should, ==, 1);
 #else /* HAVE_KIST_SUPPORT */
@@ -971,7 +971,7 @@ test_scheduler_can_use_kist(void *arg)
   clear_options();
   mocked_options.KISTSchedRunInterval = 0;
   res_should = scheduler_can_use_kist();
-  res_freq = kist_scheduler_run_interval(NULL);
+  res_freq = kist_scheduler_run_interval();
 #ifdef HAVE_KIST_SUPPORT
   tt_int_op(res_should, ==, 1);
 #else /* HAVE_KIST_SUPPORT */
@@ -984,7 +984,7 @@ test_scheduler_can_use_kist(void *arg)
   clear_options();
   mocked_options.KISTSchedRunInterval = 0;
   res_should = scheduler_can_use_kist();
-  res_freq = kist_scheduler_run_interval(NULL);
+  res_freq = kist_scheduler_run_interval();
 #ifdef HAVE_KIST_SUPPORT
   tt_int_op(res_should, ==, 1);
 #else /* HAVE_KIST_SUPPORT */
@@ -998,7 +998,7 @@ test_scheduler_can_use_kist(void *arg)
   clear_options();
   mocked_options.KISTSchedRunInterval = 0;
   res_should = scheduler_can_use_kist();
-  res_freq = kist_scheduler_run_interval(NULL);
+  res_freq = kist_scheduler_run_interval();
   tt_int_op(res_should, ==, 0);
   tt_int_op(res_freq, ==, 0);
   UNMOCK(networkstatus_get_param);
@@ -1032,7 +1032,7 @@ test_scheduler_ns_changed(void *arg)
   /* Change from vanilla to kist via consensus */
   the_scheduler = get_vanilla_scheduler();
   MOCK(networkstatus_get_param, mock_kist_networkstatus_get_param);
-  scheduler_notify_networkstatus_changed(NULL, NULL);
+  scheduler_notify_networkstatus_changed();
   UNMOCK(networkstatus_get_param);
 #ifdef HAVE_KIST_SUPPORT
   tt_ptr_op(the_scheduler, ==, get_kist_scheduler());
@@ -1043,14 +1043,14 @@ test_scheduler_ns_changed(void *arg)
   /* Change from kist to vanilla via consensus */
   the_scheduler = get_kist_scheduler();
   MOCK(networkstatus_get_param, mock_vanilla_networkstatus_get_param);
-  scheduler_notify_networkstatus_changed(NULL, NULL);
+  scheduler_notify_networkstatus_changed();
   UNMOCK(networkstatus_get_param);
   tt_ptr_op(the_scheduler, ==, get_vanilla_scheduler());
 
   /* Doesn't change when using KIST */
   the_scheduler = get_kist_scheduler();
   MOCK(networkstatus_get_param, mock_kist_networkstatus_get_param);
-  scheduler_notify_networkstatus_changed(NULL, NULL);
+  scheduler_notify_networkstatus_changed();
   UNMOCK(networkstatus_get_param);
 #ifdef HAVE_KIST_SUPPORT
   tt_ptr_op(the_scheduler, ==, get_kist_scheduler());
@@ -1061,7 +1061,7 @@ test_scheduler_ns_changed(void *arg)
   /* Doesn't change when using vanilla */
   the_scheduler = get_vanilla_scheduler();
   MOCK(networkstatus_get_param, mock_vanilla_networkstatus_get_param);
-  scheduler_notify_networkstatus_changed(NULL, NULL);
+  scheduler_notify_networkstatus_changed();
   UNMOCK(networkstatus_get_param);
   tt_ptr_op(the_scheduler, ==, get_vanilla_scheduler());