reinit per-conn token buckets on config or consensus change

changes/1830-token-buckets

@@ -0,0 +1,9 @@
+  o Major bugfixes:
+    - The PerConnBWRate and Burst config options, along with the
+      bwconnrate and bwconnburst consensus params, initialized each conn's
+      token bucket values only when the connection is established. Now
+      update them if the config options change, and update them every time
+      we get a new consensus. Otherwise we can encounter an ugly edge
+      case where we initialize an OR conn to client-level bandwidth,
+      but then later the relay joins the consensus and we leave it
+      throttled. Bugfix on 0.2.2.7-alpha; fixes bug 1830.

src/or/config.c

@@ -17,6 +17,7 @@
 #include "config.h"
 #include "connection.h"
 #include "connection_edge.h"
+#include "connection_or.h"
 #include "control.h"
 #include "cpuworker.h"
 #include "dirserv.h"
@@ -1292,6 +1293,10 @@ options_act(or_options_t *old_options)
 
     if (options->V3AuthoritativeDir && !old_options->V3AuthoritativeDir)
       init_keys();
+
+    if (options->PerConnBWRate != old_options->PerConnBWRate ||
+        options->PerConnBWBurst != old_options->PerConnBWBurst)
+      connection_or_update_token_buckets(get_connection_array(), options);
   }
 
   /* Maybe load geoip file */

src/or/connection_or.c

@@ -355,8 +355,9 @@ connection_or_digest_is_known_relay(const char *id_digest)
  * not a known relay, first check if we set PerConnBwRate/Burst, then
  * check if the consensus sets them, else default to 'big enough'.
  */
-static void connection_or_set_rate_burst(or_connection_t *conn,
-                                         or_options_t *options)
+static void
+connection_or_update_token_buckets_helper(or_connection_t *conn, int reset,
+                                          or_options_t *options)
 {
   int rate, burst; /* per-connection rate limiting params */
   if (connection_or_digest_is_known_relay(conn->identity_digest)) {
@@ -378,7 +379,29 @@ static void connection_or_set_rate_burst(or_connection_t *conn,
   }
 
   conn->bandwidthrate = rate;
-  conn->read_bucket = conn->write_bucket = conn->bandwidthburst = burst;
+  conn->bandwidthburst = burst;
+  if (reset) { /* set up the token buckets to be full */
+    conn->read_bucket = conn->write_bucket = burst;
+    return;
+  }
+  /* If the new token bucket is smaller, take out the extra tokens.
+   * (If it's larger, don't -- the buckets can grow to reach the cap.) */
+  if (conn->read_bucket > burst)
+    conn->read_bucket = burst;
+  if (conn->write_bucket > burst)
+    conn->write_bucket = burst;
+}
+
+/** Either our set of relays or our per-conn rate limits have changed.
+ * Go through all the OR connections and update their token buckets. */
+void
+connection_or_update_token_buckets(smartlist_t *conns, or_options_t *options)
+{
+  SMARTLIST_FOREACH(conns, connection_t *, conn,
+  {
+    if (connection_speaks_cells(conn))
+      connection_or_update_token_buckets_helper(TO_OR_CONN(conn), 0, options);
+  });
 }
 
 /** If we don't necessarily know the router we're connecting to, but we
@@ -392,7 +415,7 @@ connection_or_init_conn_from_address(or_connection_t *conn,
 {
   routerinfo_t *r = router_get_by_digest(id_digest);
   connection_or_set_identity_digest(conn, id_digest);
-  connection_or_set_rate_burst(conn, get_options());
+  connection_or_update_token_buckets_helper(conn, 1, get_options());
 
   conn->_base.port = port;
   tor_addr_copy(&conn->_base.addr, addr);

src/or/connection_or.h

@@ -26,6 +26,8 @@ int connection_or_flushed_some(or_connection_t *conn);
 int connection_or_finished_flushing(or_connection_t *conn);
 int connection_or_finished_connecting(or_connection_t *conn);
 int connection_or_digest_is_known_relay(const char *id_digest);
+void connection_or_update_token_buckets(smartlist_t *conns,
+                                        or_options_t *options);
 
 void connection_or_connect_failed(or_connection_t *conn,
                                   int reason, const char *msg);

src/or/networkstatus.c

@@ -14,10 +14,12 @@
 #include "circuitbuild.h"
 #include "config.h"
 #include "connection.h"
+#include "connection_or.h"
 #include "control.h"
 #include "directory.h"
 #include "dirserv.h"
 #include "dirvote.h"
+#include "main.h"
 #include "networkstatus.h"
 #include "relay.h"
 #include "router.h"
@@ -1506,6 +1508,7 @@ networkstatus_set_current_consensus(const char *consensus,
   networkstatus_t *c=NULL;
   int r, result = -1;
   time_t now = time(NULL);
+  or_options_t *options = get_options();
   char *unverified_fname = NULL, *consensus_fname = NULL;
   int flav = networkstatus_parse_flavor_name(flavor);
   const unsigned from_cache = flags & NSSET_FROM_CACHE;
@@ -1543,7 +1546,7 @@ networkstatus_set_current_consensus(const char *consensus,
   }
 
   if (flav != USABLE_CONSENSUS_FLAVOR &&
-      !directory_caches_dir_info(get_options())) {
+      !directory_caches_dir_info(options)) {
     /* This consensus is totally boring to us: we won't use it, and we won't
      * serve it.  Drop it. */
     goto done;
@@ -1678,7 +1681,7 @@ networkstatus_set_current_consensus(const char *consensus,
       download_status_failed(&consensus_dl_status[flav], 0);
   }
 
-  if (directory_caches_dir_info(get_options())) {
+  if (directory_caches_dir_info(options)) {
     dirserv_set_cached_consensus_networkstatus(consensus,
                                                flavor,
                                                &c->digests,
@@ -1691,9 +1694,13 @@ networkstatus_set_current_consensus(const char *consensus,
 
     /* XXXXNM Microdescs: needs a non-ns variant. */
     update_consensus_networkstatus_fetch_time(now);
-    dirvote_recalculate_timing(get_options(), now);
+    dirvote_recalculate_timing(options, now);
     routerstatus_list_update_named_server_map();
-    cell_ewma_set_scale_factor(get_options(), current_consensus);
+    cell_ewma_set_scale_factor(options, current_consensus);
+
+    /* XXX022 where is the right place to put this call? */
+    connection_or_update_token_buckets(get_connection_array(), options);
+
     circuit_build_times_new_consensus_params(&circ_times, current_consensus);
   }