r11637@catbus: nickm | 2007-02-05 12:41:51 -0500

Fix an XXXX012, and make circuits_pending_or_conns a static variable.  In addition to cleaning up the code, this may also resolve Bug 386 if Roger has the right intuition there.


svn:r9482

ChangeLog

@@ -64,6 +64,8 @@ Changes in version 0.1.2.7-alpha - 2007-??-??
     - Stop using the reserved ac_cv namespace in our configure script.
     - Call stat() slightly less often; use fstat() when possible.
     - Treat failure to parse resolv.conf as an error.
+    - Refactor the way we handle pending circuits when an OR connection
+      completes or fails, in an attempt to fix a rare crash bug.
 
   o Major features:
     - Weight directory requests by advertised bandwidth. Now we can

src/or/circuitbuild.c

@@ -388,8 +388,6 @@ circuit_handle_first_hop(origin_circuit_t *circ)
   return 0;
 }
 
-extern smartlist_t *circuits_pending_or_conns;
-
 /** Find any circuits that are waiting on <b>or_conn</b> to become
  * open and get them to send their create cells forward.
  *
@@ -398,25 +396,24 @@ extern smartlist_t *circuits_pending_or_conns;
 void
 circuit_n_conn_done(or_connection_t *or_conn, int status)
 {
-  smartlist_t *changed_circs;
+  smartlist_t *pending_circs;
   int err_reason = 0;
 
   log_debug(LD_CIRC,"or_conn to %s, status=%d",
             or_conn->nickname ? or_conn->nickname : "NULL", status);
 
-  if (!circuits_pending_or_conns)
-    return;
+  pending_circs = smartlist_create();
+  circuit_get_all_pending_on_or_conn(pending_circs, or_conn);
 
-  changed_circs = smartlist_create();
-
-  SMARTLIST_FOREACH(circuits_pending_or_conns, circuit_t *, circ,
-  {
-    if (circ->marked_for_close)
-      continue;
-    tor_assert(circ->state == CIRCUIT_STATE_OR_WAIT);
-    if (!circ->n_conn &&
-        !memcmp(or_conn->identity_digest, circ->n_conn_id_digest,
-                DIGEST_LEN)) {
+  SMARTLIST_FOREACH(pending_circs, circuit_t *, circ,
+    {
+      /* This check is redundant wrt get_all_pending_on_or_conn, but I'm
+       * leaving them in in case it's possible for the status of a circuit to
+       * change as we're going down the list. */
+      if (circ->marked_for_close || circ->n_conn ||
+          circ->state != CIRCUIT_STATE_OR_WAIT ||
+          memcmp(or_conn->identity_digest, circ->n_conn_id_digest, DIGEST_LEN))
+        continue;
       if (!status) { /* or_conn failed; close circ */
         log_info(LD_CIRC,"or_conn failed. Closing circ.");
         circuit_mark_for_close(circ, END_CIRC_REASON_OR_CONN_CLOSED);
@@ -445,18 +442,11 @@ circuit_n_conn_done(or_connection_t *or_conn, int status)
           continue;
         }
         tor_free(circ->onionskin);
-        /* We don't want to change circ's state here, since the act
-         * of doing that modifies the circuits_pending_or_conns list
-         * that we're looping through right now. So collect a list of
-         * circs to change their state when we're done. */
-        smartlist_add(changed_circs, circ);
+        circuit_set_state(circ, CIRCUIT_STATE_OPEN);
       }
-    }
-  });
+    });
 
-  SMARTLIST_FOREACH(changed_circs, circuit_t *, circ,
-    circuit_set_state(circ, CIRCUIT_STATE_OPEN));
-  smartlist_free(changed_circs);
+  smartlist_free(pending_circs);
 }
 
 /** Find a new circid that isn't currently in use on the circ->n_conn

src/or/circuitlist.c

@@ -21,7 +21,7 @@ const char circuitlist_c_id[] =
 circuit_t *global_circuitlist=NULL;
 
 /** A list of all the circuits in CIRCUIT_STATE_OR_WAIT. */
-smartlist_t *circuits_pending_or_conns=NULL;
+static smartlist_t *circuits_pending_or_conns=NULL;
 
 static void circuit_free(circuit_t *circ);
 static void circuit_free_cpath(crypt_path_t *cpath);
@@ -165,15 +165,14 @@ circuit_set_state(circuit_t *circ, int state)
   tor_assert(circ);
   if (state == circ->state)
     return;
+  if (!circuits_pending_or_conns)
+    circuits_pending_or_conns = smartlist_create();
   if (circ->state == CIRCUIT_STATE_OR_WAIT) {
     /* remove from waiting-circuit list. */
-    if (circuits_pending_or_conns)
-      smartlist_remove(circuits_pending_or_conns, circ);
+    smartlist_remove(circuits_pending_or_conns, circ);
   }
   if (state == CIRCUIT_STATE_OR_WAIT) {
     /* add to waiting-circuit list. */
-    if (!circuits_pending_or_conns)
-      circuits_pending_or_conns = smartlist_create();
     smartlist_add(circuits_pending_or_conns, circ);
   }
   circ->state = state;
@@ -194,6 +193,45 @@ circuit_add(circuit_t *circ)
   }
 }
 
+/** Append to <b>out</b> the number of circuits in state OR_WAIT, waiting for
+ * the given connection. */
+void
+circuit_get_all_pending_on_or_conn(smartlist_t *out, or_connection_t *or_conn)
+{
+  tor_assert(out);
+  tor_assert(or_conn);
+
+  if (!circuits_pending_or_conns)
+    return;
+
+  SMARTLIST_FOREACH(circuits_pending_or_conns, circuit_t *, circ,
+  {
+    if (circ->marked_for_close)
+      continue;
+    tor_assert(circ->state == CIRCUIT_STATE_OR_WAIT);
+    if (!circ->n_conn &&
+        !memcmp(or_conn->identity_digest, circ->n_conn_id_digest,
+                DIGEST_LEN)) {
+      smartlist_add(out, circ);
+    }
+  });
+}
+
+/** Return the number of circuits in state OR_WAIT, waiting for the given
+ * connection.  */
+int
+circuit_count_pending_on_or_conn(or_connection_t *or_conn)
+{
+  int cnt;
+  smartlist_t *sl = smartlist_create();
+  circuit_get_all_pending_on_or_conn(sl, or_conn);
+  cnt = smartlist_len(sl);
+  smartlist_free(sl);
+  log_debug(LD_CIRC,"or_conn to %s, %d pending circs",
+            or_conn->nickname ? or_conn->nickname : "NULL", cnt);
+  return cnt;
+}
+
 /** Detach from the global circuit list, and deallocate, all
  * circuits that have been marked for close.
  */

src/or/connection_or.c

@@ -779,37 +779,6 @@ connection_or_send_destroy(uint16_t circ_id, or_connection_t *conn, int reason)
   return 0;
 }
 
-/* XXXX012 This global is getting _too_ global. -NM */
-extern smartlist_t *circuits_pending_or_conns;
-
-/** Count number of pending circs on an or_conn */
-int
-connection_or_count_pending_circs(or_connection_t *or_conn)
-{
-  int cnt = 0;
-
-  if (!circuits_pending_or_conns)
-    return 0;
-
-  tor_assert(or_conn);
-
-  SMARTLIST_FOREACH(circuits_pending_or_conns, circuit_t *, circ,
-  {
-    if (circ->marked_for_close)
-      continue;
-    tor_assert(circ->state == CIRCUIT_STATE_OR_WAIT);
-    if (!circ->n_conn &&
-        !memcmp(or_conn->identity_digest, circ->n_conn_id_digest,
-                DIGEST_LEN)) {
-      cnt++;
-    }
-  });
-
-  log_debug(LD_CIRC,"or_conn to %s, %d pending circs",
-            or_conn->nickname ? or_conn->nickname : "NULL", cnt);
-  return cnt;
-}
-
 /** DOCDOC */
 #define BUF_FULLNESS_THRESHOLD (128*1024)
 /** DOCDOC */

src/or/control.c

@@ -3356,7 +3356,7 @@ control_event_or_conn_status(or_connection_t *conn,or_conn_status_event_t tp,
         log_warn(LD_BUG, "Unrecognized status code %d", (int)tp);
         return 0;
       }
-    ncircs = connection_or_count_pending_circs(conn);
+    ncircs = circuit_count_pending_on_or_conn(conn);
     ncircs += conn->n_circuits;
     if (ncircs && (tp == OR_CONN_EVENT_FAILED || tp == OR_CONN_EVENT_CLOSED)) {
         tor_snprintf(ncircs_buf, sizeof(ncircs_buf), "%sNCIRCS=%d",

src/or/or.h

@@ -1989,6 +1989,9 @@ void circuit_expire_all_dirty_circs(void);
 void _circuit_mark_for_close(circuit_t *circ, int reason,
                              int line, const char *file);
 int circuit_get_cpath_len(origin_circuit_t *circ);
+void circuit_get_all_pending_on_or_conn(smartlist_t *out,
+                                        or_connection_t *or_conn);
+int circuit_count_pending_on_or_conn(or_connection_t *or_conn);
 
 #define circuit_mark_for_close(c, reason)                               \
   _circuit_mark_for_close((c), (reason), __LINE__, _SHORT_FILE_)
@@ -2250,7 +2253,6 @@ void connection_or_write_cell_to_buf(const cell_t *cell,
                                      or_connection_t *conn);
 int connection_or_send_destroy(uint16_t circ_id, or_connection_t *conn,
                                int reason);
-int connection_or_count_pending_circs(or_connection_t *or_conn);
 
 /********************************* control.c ***************************/