Experimentally decouple the main body of circuit_mark_for_close

changes/decouple_circuit_mark

@@ -0,0 +1,6 @@
+  o Code simplification and refactoring:
+     - Extract the more complicated parts of circuit_mark_for_close into
+       a new function run periodically before connections are freed.
+       This change removes more than half of the functions currently
+       in the "blob".
+       Closes ticket #17218.

src/or/circuitlist.c

@@ -44,11 +44,16 @@ static smartlist_t *global_circuitlist = NULL;
 /** A list of all the circuits in CIRCUIT_STATE_CHAN_WAIT. */
 static smartlist_t *circuits_pending_chans = NULL;
 
+/** A list of all the circuits that have been marked with circuit_mark_for_close
+ * and which are waiting for circuit_about_to_free(). */
+static smartlist_t *circuits_pending_close = NULL;
+
 static void circuit_free_cpath_node(crypt_path_t *victim);
 static void cpath_ref_decref(crypt_path_reference_t *cpath_ref);
 //static void circuit_set_rend_token(or_circuit_t *circ, int is_rend_circ,
 //                                   const uint8_t *token);
 static void circuit_clear_rend_token(or_circuit_t *circ);
+static void circuit_about_to_free(circuit_t *circ);
 
 /********* END VARIABLES ************/
 
@@ -451,16 +456,27 @@ circuit_count_pending_on_channel(channel_t *chan)
 void
 circuit_close_all_marked(void)
 {
+  if (circuits_pending_close == NULL)
+    return;
+
   smartlist_t *lst = circuit_get_global_list();
-  SMARTLIST_FOREACH_BEGIN(lst, circuit_t *, circ) {
+  SMARTLIST_FOREACH_BEGIN(circuits_pending_close, circuit_t *, circ) {
-    /* Fix up index if SMARTLIST_DEL_CURRENT just moved this one. */
+    tor_assert(circ->marked_for_close);
-    circ->global_circuitlist_idx = circ_sl_idx;
+
-    if (circ->marked_for_close) {
+    /* Remove it from the circuit list. */
-      circ->global_circuitlist_idx = -1;
+    int idx = circ->global_circuitlist_idx;
-      circuit_free(circ);
+    smartlist_del(lst, idx);
-      SMARTLIST_DEL_CURRENT(lst, circ);
+    if (idx < smartlist_len(lst)) {
+      circuit_t *replacement = smartlist_get(lst, idx);
+      replacement->global_circuitlist_idx = idx;
     }
+    circ->global_circuitlist_idx = -1;
+
+    circuit_about_to_free(circ);
+    circuit_free(circ);
   } SMARTLIST_FOREACH_END(circ);
+
+  smartlist_clear(circuits_pending_close);
 }
 
 /** Return the head of the global linked list of circuits. */
@@ -1703,6 +1719,39 @@ circuit_mark_for_close_, (circuit_t *circ, int reason, int line,
     reason = END_CIRC_REASON_NONE;
   }
 
+  circ->marked_for_close = line;
+  circ->marked_for_close_file = file;
+  circ->marked_for_close_reason = reason;
+  circ->marked_for_close_orig_reason = orig_reason;
+
+  if (!CIRCUIT_IS_ORIGIN(circ)) {
+    or_circuit_t *or_circ = TO_OR_CIRCUIT(circ);
+    if (or_circ->rend_splice) {
+      if (!or_circ->rend_splice->base_.marked_for_close) {
+        /* do this after marking this circuit, to avoid infinite recursion. */
+        circuit_mark_for_close(TO_CIRCUIT(or_circ->rend_splice), reason);
+      }
+      or_circ->rend_splice = NULL;
+    }
+  }
+
+  if (circuits_pending_close == NULL)
+    circuits_pending_close = smartlist_new();
+
+  smartlist_add(circuits_pending_close, circ);
+}
+
+/** Called immediately before freeing a marked circuit <b>circ</b>.
+ * Disconnects the circuit from other data structures, launches events
+ * as appropriate, and performs other housekeeping.
+ */
+static void
+circuit_about_to_free(circuit_t *circ)
+{
+
+  int reason = circ->marked_for_close_reason;
+  int orig_reason = circ->marked_for_close_orig_reason;
+
   if (circ->state == CIRCUIT_STATE_ONIONSKIN_PENDING) {
     onion_pending_remove(TO_OR_CIRCUIT(circ));
   }
@@ -1726,6 +1775,7 @@ circuit_mark_for_close_, (circuit_t *circ, int reason, int line,
      (circ->state == CIRCUIT_STATE_OPEN)?CIRC_EVENT_CLOSED:CIRC_EVENT_FAILED,
      orig_reason);
   }
+
   if (circ->purpose == CIRCUIT_PURPOSE_C_INTRODUCE_ACK_WAIT) {
     origin_circuit_t *ocirc = TO_ORIGIN_CIRCUIT(circ);
     int timed_out = (reason == END_CIRC_REASON_TIMEOUT);
@@ -1811,19 +1861,6 @@ circuit_mark_for_close_, (circuit_t *circ, int reason, int line,
     ocirc->p_streams = NULL;
   }
 
-  circ->marked_for_close = line;
-  circ->marked_for_close_file = file;
-
-  if (!CIRCUIT_IS_ORIGIN(circ)) {
-    or_circuit_t *or_circ = TO_OR_CIRCUIT(circ);
-    if (or_circ->rend_splice) {
-      if (!or_circ->rend_splice->base_.marked_for_close) {
-        /* do this after marking this circuit, to avoid infinite recursion. */
-        circuit_mark_for_close(TO_CIRCUIT(or_circ->rend_splice), reason);
-      }
-      or_circ->rend_splice = NULL;
-    }
-  }
 }
 
 /** Given a marked circuit <b>circ</b>, aggressively free its cell queues to

src/or/or.h

@@ -2892,6 +2892,14 @@ typedef struct circuit_t {
                               * where this circuit was marked.) */
   const char *marked_for_close_file; /**< For debugging: in which file was this
                                       * circuit marked for close? */
+  /** For what reason (See END_CIRC_REASON...) is this circuit being closed?
+   * This field is set in circuit_mark_for_close and used later in
+   * circuit_about_to_free. */
+  uint16_t marked_for_close_reason;
+  /** As marked_for_close_reason, but reflects the underlying reason for
+   * closing this circuit.
+   */
+  uint16_t marked_for_close_orig_reason;
 
   /** Unique ID for measuring tunneled network status requests. */
   uint64_t dirreq_id;