Replace last_added_nonpadding with last_had_circuits

The point of the "idle timeout" for connections is to kill the
connection a while after it has no more circuits.  But using "last
added a non-padding cell" as a proxy for that is wrong, since if the
last circuit is closed from the other side of the connection, we
will not have sent anything on that connection since well before the
last circuit closed.

This is part of fixing 6799.

When applied to 0.2.5, it is also a fix for 12023.

changes/bug6799

@@ -1,13 +1,20 @@
   o Major features:
 
-    - Increate the base amount of time that a canonical connection
+    - Increase the base amount of time that a canonical connection
       (one that we have made to a known OR) is allowed to stay open
       from a 3 minutes to 15 minutes.  This leaks less information
       about when circuits have closed, and avoids unnecessary overhead
       from renegotiating connections. Part of a fix for ticket 6799.
 
-    - Instead of closing connections at a fixed interval after their
-      last circuit closed, randomly add up to 50% to each connection's
-      maximum timout. This makes it harder to tell when the last
+    - Instead of closing connections after they have been idle for a
+      fixed interval, randomly add up to 50% to each connection's
+      maximum timeout. This makes it harder to tell when the last
       circuit closed by looking at when a connection closes. Part of a
       fix for ticket 6799.
+
+    - Base connection idleness tests on the actual time elapsed since
+      the connection last had circuits, not on the time when we last
+      added non-padding. This also makes it harder to tell when the last
+      circuit closed by looking at when a connection closes. Part of a
+      fix for ticket 6799.
+      Incidentally fixes bug 12023; bugfix on 0.2.5.1-alpha.

src/or/channel.c

@@ -117,7 +117,9 @@ HT_GENERATE(channel_idmap, channel_idmap_entry_s, node, channel_idmap_hash,
 
 static cell_queue_entry_t * cell_queue_entry_dup(cell_queue_entry_t *q);
 static void cell_queue_entry_free(cell_queue_entry_t *q, int handed_off);
+#if 0
 static int cell_queue_entry_is_padding(cell_queue_entry_t *q);
+#endif
 static cell_queue_entry_t *
 cell_queue_entry_new_fixed(cell_t *cell);
 static cell_queue_entry_t *
@@ -729,7 +731,7 @@ channel_init(channel_t *chan)
   chan->global_identifier = n_channels_allocated++;
 
   /* Init timestamp */
-  chan->timestamp_last_added_nonpadding = time(NULL);
+  chan->timestamp_last_had_circuits = time(NULL);
 
   /* Init next_circ_id */
   chan->next_circ_id = crypto_rand_int(1 << 15);
@@ -1597,6 +1599,7 @@ cell_queue_entry_free(cell_queue_entry_t *q, int handed_off)
   tor_free(q);
 }
 
+#if 0
 /**
  * Check whether a cell queue entry is padding; this is a helper function
  * for channel_write_cell_queue_entry()
@@ -1625,6 +1628,7 @@ cell_queue_entry_is_padding(cell_queue_entry_t *q)
 
   return 0;
 }
+#endif
 
 /**
  * Allocate a new cell queue entry for a fixed-size cell
@@ -1683,11 +1687,6 @@ channel_write_cell_queue_entry(channel_t *chan, cell_queue_entry_t *q)
              chan->state == CHANNEL_STATE_OPEN ||
              chan->state == CHANNEL_STATE_MAINT);
 
-  /* Increment the timestamp unless it's padding */
-  if (!cell_queue_entry_is_padding(q)) {
-    chan->timestamp_last_added_nonpadding = approx_time();
-  }
-
   /* Can we send it right out?  If so, try */
   if (TOR_SIMPLEQ_EMPTY(&chan->outgoing_queue) &&
       chan->state == CHANNEL_STATE_OPEN) {

src/or/channel.h

@@ -187,8 +187,10 @@ struct channel_s {
   time_t timestamp_recv; /* Cell received from lower layer */
   time_t timestamp_xmit; /* Cell sent to lower layer */
 
-  /* Timestamp for relay.c */
-  time_t timestamp_last_added_nonpadding;
+  /** Timestamp for run_connection_housekeeping(). We update this once a
+   * second when we run housekeeping and find a circuit on this channel, and
+   * whenever we add a circuit to the channel. */
+  time_t timestamp_last_had_circuits;
 
   /** Unique ID for measuring direct network status requests;vtunneled ones
    * come over a circuit_t, which has a dirreq_id field as well, but is a

src/or/circuitlist.c

@@ -217,8 +217,11 @@ circuit_set_p_circid_chan(or_circuit_t *circ, circid_t id,
   circuit_set_circid_chan_helper(TO_CIRCUIT(circ), CELL_DIRECTION_IN,
                                  id, chan);
 
-  if (chan)
+  if (chan) {
     tor_assert(bool_eq(circ->p_chan_cells.n, circ->next_active_on_p_chan));
+
+    chan->timestamp_last_had_circuits = approx_time();
+  }
 }
 
 /** Set the n_conn field of a circuit <b>circ</b>, along
@@ -230,8 +233,11 @@ circuit_set_n_circid_chan(circuit_t *circ, circid_t id,
 {
   circuit_set_circid_chan_helper(circ, CELL_DIRECTION_OUT, id, chan);
 
-  if (chan)
+  if (chan) {
     tor_assert(bool_eq(circ->n_chan_cells.n, circ->next_active_on_n_chan));
+
+    chan->timestamp_last_had_circuits = approx_time();
+  }
 }
 
 /** Change the state of <b>circ</b> to <b>state</b>, adding it to or removing

src/or/connection.c

@@ -249,8 +249,6 @@ dir_connection_new(int socket_family)
 /** Allocate and return a new or_connection_t, initialized as by
  * connection_init().
  *
- * Set timestamp_last_added_nonpadding to now.
- *
  * Assign a pseudorandom next_circ_id between 0 and 2**15.
  *
  * Initialize active_circuit_pqueue.
@@ -264,8 +262,6 @@ or_connection_new(int socket_family)
   time_t now = time(NULL);
   connection_init(now, TO_CONN(or_conn), CONN_TYPE_OR, socket_family);
 
-  or_conn->timestamp_last_added_nonpadding = time(NULL);
-
   connection_or_set_canonical(or_conn, 0);
 
   return or_conn;

src/or/connection_or.c

@@ -1929,9 +1929,6 @@ connection_or_write_cell_to_buf(const cell_t *cell, or_connection_t *conn)
 
   if (conn->base_.state == OR_CONN_STATE_OR_HANDSHAKING_V3)
     or_handshake_state_record_cell(conn, conn->handshake_state, cell, 0);
-
-  if (cell->command != CELL_PADDING)
-    conn->timestamp_last_added_nonpadding = approx_time();
 }
 
 /** Pack a variable-length <b>cell</b> into wire-format, and write it onto
@@ -1952,8 +1949,6 @@ connection_or_write_var_cell_to_buf(const var_cell_t *cell,
                           cell->payload_len, TO_CONN(conn));
   if (conn->base_.state == OR_CONN_STATE_OR_HANDSHAKING_V3)
     or_handshake_state_record_var_cell(conn, conn->handshake_state, cell, 0);
-  if (cell->command != CELL_PADDING)
-    conn->timestamp_last_added_nonpadding = approx_time();
 
   /* Touch the channel's active timestamp if there is one */
   if (conn->chan)

src/or/main.c

@@ -1003,6 +1003,8 @@ run_connection_housekeeping(int i, time_t now)
   connection_t *conn = smartlist_get(connection_array, i);
   const or_options_t *options = get_options();
   or_connection_t *or_conn;
+  channel_t *chan = NULL;
+  int have_any_circuits;
   int past_keepalive =
     now >= conn->timestamp_lastwritten + options->KeepalivePeriod;
 
@@ -1050,8 +1052,19 @@ run_connection_housekeeping(int i, time_t now)
   tor_assert(conn->outbuf);
 #endif
 
+  chan = TLS_CHAN_TO_BASE(or_conn->chan);
+  tor_assert(chan);
+
+
+  if (channel_num_circuits(chan) != 0) {
+    have_any_circuits = 1;
+    chan->timestamp_last_had_circuits = now;
+  } else {
+    have_any_circuits = 0;
+  }
+
   if (channel_is_bad_for_new_circs(TLS_CHAN_TO_BASE(or_conn->chan)) &&
-      !connection_or_get_num_circuits(or_conn)) {
+      ! have_any_circuits) {
     /* It's bad for new circuits, and has no unmarked circuits on it:
      * mark it now. */
     log_info(LD_OR,
@@ -1070,19 +1083,21 @@ run_connection_housekeeping(int i, time_t now)
       connection_or_close_normally(TO_OR_CONN(conn), 0);
     }
   } else if (we_are_hibernating() &&
-             !connection_or_get_num_circuits(or_conn) &&
+             ! have_any_circuits &&
              !connection_get_outbuf_len(conn)) {
     /* We're hibernating, there's no circuits, and nothing to flush.*/
     log_info(LD_OR,"Expiring non-used OR connection to fd %d (%s:%d) "
              "[Hibernating or exiting].",
              (int)conn->s,conn->address, conn->port);
     connection_or_close_normally(TO_OR_CONN(conn), 1);
-  } else if (!connection_or_get_num_circuits(or_conn) &&
-             now >= or_conn->timestamp_last_added_nonpadding +
-                             or_conn->idle_timeout) {
+  } else if (!have_any_circuits &&
+             now - or_conn->idle_timeout >= chan->timestamp_last_had_circuits) {
     log_info(LD_OR,"Expiring non-used OR connection to fd %d (%s:%d) "
-             "[idle %d].", (int)conn->s,conn->address, conn->port,
-             (int)(now - or_conn->timestamp_last_added_nonpadding));
+             "[no circuits for %d; timeout %d; %scanonical].",
+             (int)conn->s, conn->address, conn->port,
+             (int)(now - chan->timestamp_last_had_circuits),
+             or_conn->idle_timeout,
+             or_conn->is_canonical ? "" : "non");
     connection_or_close_normally(TO_OR_CONN(conn), 0);
   } else if (
       now >= or_conn->timestamp_lastempty + options->KeepalivePeriod*10 &&

src/or/or.h

@@ -1432,8 +1432,6 @@ typedef struct or_connection_t {
                                           * up, state information to do so. */
 
   time_t timestamp_lastempty; /**< When was the outbuf last completely empty?*/
-  time_t timestamp_last_added_nonpadding; /** When did we last add a
-                                           * non-padding cell to the outbuf? */
 
   /* bandwidth* and *_bucket only used by ORs in OPEN state: */
   int bandwidthrate; /**< Bytes/s added to the bucket. (OPEN ORs only.) */