commit 6057fb2f5bdda80b9056a750261f60ca2c960283 Author: teor (Tim Wilson-Brown) teor2345@gmail.com Date: Sat Mar 26 11:08:39 2016 +1100
Clarify excess consensus connection cleanup by adding comments
Comment-only change --- src/or/directory.c | 44 +++++++++++++++++++++++++++++++++++--------- src/or/main.c | 10 ++++++++-- 2 files changed, 43 insertions(+), 11 deletions(-)
diff --git a/src/or/directory.c b/src/or/directory.c index 99091b5..9139dbc 100644 --- a/src/or/directory.c +++ b/src/or/directory.c @@ -1161,6 +1161,8 @@ directory_initiate_command_rend(const tor_addr_port_t *or_addr_port, conn->base_.state = DIR_CONN_STATE_CLIENT_SENDING; /* fall through */ case 0: + /* Close this connection if there's another consensus connection + * downloading (during bootstrap), or connecting (after bootstrap). */ if (connection_dir_close_consensus_conn_if_extra(conn)) { return; } @@ -1211,8 +1213,8 @@ directory_initiate_command_rend(const tor_addr_port_t *or_addr_port, connection_mark_for_close(TO_CONN(conn)); return; } - /* XXX the below line is suspicious and uncommented. does it close all - * consensus fetches if we've already bootstrapped? investigate. */ + /* Close this connection if there's another consensus connection + * downloading (during bootstrap), or connecting (after bootstrap). */ if (connection_dir_close_consensus_conn_if_extra(conn)) { return; } @@ -3689,11 +3691,16 @@ connection_dir_avoid_extra_connection_for_purpose(unsigned int purpose) return 0; }
-/* Check if we have excess consensus download connection attempts, and close - * conn: +/* Check if we have more than one consensus download connection attempt, and + * close conn: * - if we don't have a consensus, and we're downloading a consensus, and conn - * is not downloading a consensus yet, close it; - * - if we do have a consensus, conn is excess, close it. */ + * is not downloading a consensus yet; + * - if we do have a consensus, and there's more than one consensus connection. + * + * Post-bootstrap consensus connection attempts are initiated one at a time. + * So this function won't close any consensus connection attempts that + * are initiated after bootstrap. + */ int connection_dir_close_consensus_conn_if_extra(dir_connection_t *conn) { @@ -3710,6 +3717,10 @@ connection_dir_close_consensus_conn_if_extra(dir_connection_t *conn) return 0; }
+ /* Only close this connection if there's another consensus connection + * downloading (during bootstrap), or connecting (after bootstrap). + * Post-bootstrap consensus connection attempts won't be closed, because + * they only occur one at a time. */ if (!connection_dir_would_close_consensus_conn_helper()) { return 0; } @@ -3733,15 +3744,28 @@ connection_dir_close_consensus_conn_if_extra(dir_connection_t *conn) return 0; }
-/* Check if we have excess consensus download connection attempts, and close - * them: +/* Clean up excess consensus download connection attempts. + * During bootstrap, or when the bootstrap consensus has just been downloaded, + * if we have more than one active consensus connection: * - if we don't have a consensus, and we're downloading a consensus, keep an * earlier connection, or a connection to a fallback directory, and close * all other connections; - * - if we do have a consensus, close all connections: they are all excess. */ + * - if we have just downloaded the bootstrap consensus, and have other + * consensus connections left over, close all of them. + * + * Post-bootstrap consensus connection attempts are initiated one at a time. + * So this function won't close any consensus connection attempts that + * are initiated after bootstrap. + */ void connection_dir_close_extra_consensus_conns(void) { + /* Only cleanup connections if there is more than one consensus connection, + * and at least one of those connections is already downloading + * (during bootstrap), or connecting (just after the bootstrap consensus is + * downloaded). + * Post-bootstrap consensus connection attempts won't be cleaned up, because + * they only occur one at a time. */ if (!connection_dir_would_close_consensus_conn_helper()) { return; } @@ -3833,6 +3857,8 @@ connection_dir_finished_connecting(dir_connection_t *conn) log_debug(LD_HTTP,"Dir connection to router %s:%u established.", conn->base_.address,conn->base_.port);
+ /* Close this connection if there's another consensus connection + * downloading (during bootstrap), or connecting (after bootstrap). */ if (connection_dir_close_consensus_conn_if_extra(conn)) { return -1; } diff --git a/src/or/main.c b/src/or/main.c index 00768ac..0bd3527 100644 --- a/src/or/main.c +++ b/src/or/main.c @@ -1485,8 +1485,14 @@ run_scheduled_events(time_t now) }
/* 2d. Cleanup excess consensus bootstrap connections every second. - * connection_dir_close_consensus_conn_if_extra() will close connections - * that are clearly excess, but this check is more thorough. */ + * connection_dir_close_consensus_conn_if_extra() closes some connections + * that are clearly excess, but this check is more thorough. + * This only closes connections if there is more than one consensus + * connection, and at least one of those connections is already downloading + * (during bootstrap), or connecting (just after the bootstrap consensus is + * downloaded). + * It won't close any consensus connections initiated after bootstrap, + * because those attempts are made one at a time. */ connection_dir_close_extra_consensus_conns();
/* 3a. Every second, we examine pending circuits and prune the