From 1c0c0022d8199a9b998a0c4e13a85a382abf7fb0 Mon Sep 17 00:00:00 2001 From: Andrea Shepard Date: Thu, 30 Jun 2016 10:26:13 +0000 Subject: [PATCH 01/17] Compute thresholds for OOS from ConnLimit_ when setting options --- src/or/config.c | 26 ++++++++++++++++++++++++++ src/or/or.h | 4 ++++ 2 files changed, 30 insertions(+) diff --git a/src/or/config.c b/src/or/config.c index 3fab3b3ec8..33c80e52b0 100644 --- a/src/or/config.c +++ b/src/or/config.c @@ -1334,6 +1334,32 @@ options_act_reversible(const or_options_t *old_options, char **msg) connection_mark_for_close(conn); } }); + + if (set_conn_limit) { + /* + * If we adjusted the conn limit, recompute the OOS threshold too + * + * How many possible sockets to keep in reserve? If we have lots of + * possible sockets, keep this below a limit and set ConnLimit_high_thresh + * very close to ConnLimit_, but if ConnLimit_ is low, shrink it in + * proportion. + * + * Somewhat arbitrarily, set socks_in_reserve to 5% of ConnLimit_, but + * cap it at 64. + */ + int socks_in_reserve = options->ConnLimit_ / 20; + if (socks_in_reserve > 64) socks_in_reserve = 64; + + options->ConnLimit_high_thresh = options->ConnLimit_ - socks_in_reserve; + options->ConnLimit_low_thresh = (options->ConnLimit_ / 4) * 3; + log_info(LD_GENERAL, + "Recomputed OOS thresholds: ConnLimit %d, ConnLimit_ %d, " + "ConnLimit_high_thresh %d, ConnLimit_low_thresh %d", + options->ConnLimit, options->ConnLimit_, + options->ConnLimit_high_thresh, + options->ConnLimit_low_thresh); + } + goto done; rollback: diff --git a/src/or/or.h b/src/or/or.h index 1bac43c20a..262085ea62 100644 --- a/src/or/or.h +++ b/src/or/or.h @@ -3699,6 +3699,10 @@ typedef struct { int ConnLimit; /**< Demanded minimum number of simultaneous connections. */ int ConnLimit_; /**< Maximum allowed number of simultaneous connections. */ + int ConnLimit_high_thresh; /**< start trying to lower socket usage if we + * have this many. */ + int ConnLimit_low_thresh; /**< try to get down to here after socket + * exhaustion. */ int RunAsDaemon; /**< If true, run in the background. (Unix only) */ int FascistFirewall; /**< Whether to prefer ORs reachable on open ports. */ smartlist_t *FirewallPorts; /**< Which ports our firewall allows From 34d9d02150fc41107887c27900a62c4552578789 Mon Sep 17 00:00:00 2001 From: Andrea Shepard Date: Thu, 30 Jun 2016 10:43:44 +0000 Subject: [PATCH 02/17] Stub out connection_handle_oos() and call it from places we can change the socket count or thresholds --- src/or/config.c | 3 ++ src/or/connection.c | 108 +++++++++++++++++++++++++++++++++++++++++++- src/or/connection.h | 2 + 3 files changed, 112 insertions(+), 1 deletion(-) diff --git a/src/or/config.c b/src/or/config.c index 33c80e52b0..f7a7a534b7 100644 --- a/src/or/config.c +++ b/src/or/config.c @@ -1358,6 +1358,9 @@ options_act_reversible(const or_options_t *old_options, char **msg) options->ConnLimit, options->ConnLimit_, options->ConnLimit_high_thresh, options->ConnLimit_low_thresh); + + /* Give the OOS handler a chance with the new thresholds */ + connection_handle_oos(get_n_open_sockets(), 0); } goto done; diff --git a/src/or/connection.c b/src/or/connection.c index 68e442df54..3939531876 100644 --- a/src/or/connection.c +++ b/src/or/connection.c @@ -1090,6 +1090,7 @@ connection_listener_new(const struct sockaddr *listensockaddr, int start_reading = 0; static int global_next_session_group = SESSION_GROUP_FIRST_AUTO; tor_addr_t addr; + int exhaustion = 0; if (listensockaddr->sa_family == AF_INET || listensockaddr->sa_family == AF_INET6) { @@ -1108,6 +1109,11 @@ connection_listener_new(const struct sockaddr *listensockaddr, int e = tor_socket_errno(s); if (ERRNO_IS_RESOURCE_LIMIT(e)) { warn_too_many_conns(); + /* + * We'll call the OOS handler at the error exit, so set the + * exhaustion flag for it. + */ + exhaustion = 1; } else { log_warn(LD_NET, "Socket creation failed: %s", tor_socket_strerror(e)); @@ -1226,6 +1232,11 @@ connection_listener_new(const struct sockaddr *listensockaddr, int e = tor_socket_errno(s); if (ERRNO_IS_RESOURCE_LIMIT(e)) { warn_too_many_conns(); + /* + * We'll call the OOS handler at the error exit, so set the + * exhaustion flag for it. + */ + exhaustion = 1; } else { log_warn(LD_NET,"Socket creation failed: %s.", strerror(e)); } @@ -1344,6 +1355,12 @@ connection_listener_new(const struct sockaddr *listensockaddr, dnsserv_configure_listener(conn); } + /* + * Normal exit; call the OOS handler since connection count just changed; + * the exhaustion flag will always be zero here though. + */ + connection_handle_oos(get_n_open_sockets(), 0); + return conn; err: @@ -1352,6 +1369,9 @@ connection_listener_new(const struct sockaddr *listensockaddr, if (conn) connection_free(conn); + /* Call the OOS handler, indicate if we saw an exhaustion-related error */ + connection_handle_oos(get_n_open_sockets(), exhaustion); + return NULL; } @@ -1442,21 +1462,34 @@ connection_handle_listener_read(connection_t *conn, int new_type) if (!SOCKET_OK(news)) { /* accept() error */ int e = tor_socket_errno(conn->s); if (ERRNO_IS_ACCEPT_EAGAIN(e)) { - return 0; /* they hung up before we could accept(). that's fine. */ + /* + * they hung up before we could accept(). that's fine. + * + * give the OOS handler a chance to run though + */ + connection_handle_oos(get_n_open_sockets(), 0); + return 0; } else if (ERRNO_IS_RESOURCE_LIMIT(e)) { warn_too_many_conns(); + /* Exhaustion; tell the OOS handler */ + connection_handle_oos(get_n_open_sockets(), 1); return 0; } /* else there was a real error. */ log_warn(LD_NET,"accept() failed: %s. Closing listener.", tor_socket_strerror(e)); connection_mark_for_close(conn); + /* Tell the OOS handler about this too */ + connection_handle_oos(get_n_open_sockets(), 0); return -1; } log_debug(LD_NET, "Connection accepted on socket %d (child of fd %d).", (int)news,(int)conn->s); + /* We accepted a new conn; run OOS handler */ + connection_handle_oos(get_n_open_sockets(), 0); + if (make_socket_reuseable(news) < 0) { if (tor_socket_errno(news) == EINVAL) { /* This can happen on OSX if we get a badly timed shutdown. */ @@ -1661,12 +1694,18 @@ connection_connect_sockaddr,(connection_t *conn, s = tor_open_socket_nonblocking(protocol_family, SOCK_STREAM, proto); if (! SOCKET_OK(s)) { + /* + * Early OOS handler calls; it matters if it's an exhaustion-related + * error or not. + */ *socket_error = tor_socket_errno(s); if (ERRNO_IS_RESOURCE_LIMIT(*socket_error)) { warn_too_many_conns(); + connection_handle_oos(get_n_open_sockets(), 1); } else { log_warn(LD_NET,"Error creating network socket: %s", tor_socket_strerror(*socket_error)); + connection_handle_oos(get_n_open_sockets(), 0); } return -1; } @@ -1676,6 +1715,13 @@ connection_connect_sockaddr,(connection_t *conn, tor_socket_strerror(errno)); } + /* + * We've got the socket open; give the OOS handler a chance to check + * against configuured maximum socket number, but tell it no exhaustion + * failure. + */ + connection_handle_oos(get_n_open_sockets(), 0); + if (bindaddr && bind(s, bindaddr, bindaddr_len) < 0) { *socket_error = tor_socket_errno(s); log_warn(LD_NET,"Error binding network socket: %s", @@ -4454,6 +4500,66 @@ connection_reached_eof(connection_t *conn) } } +/** Out-of-Sockets handler; n_socks is the current number of open + * sockets, and failed is non-zero if a socket exhaustion related + * error immediately preceded this call. This is where to do + * circuit-killing heuristics as needed. + */ +void +connection_handle_oos(int n_socks, int failed) +{ + int target_n_socks = 0; + + /* Sanity-check args */ + tor_assert(n_socks >= 0); + + /* + * Make some log noise; keep it at debug level since this gets a chance + * to run on every connection attempt. + */ + log_debug(LD_NET, + "Running the OOS handler (%d open sockets, %s)", + n_socks, (failed != 0) ? "exhaustion seen" : "no exhaustion"); + + /* + * Check if we're really handling an OOS condition, and if so decide how + * many sockets we want to get down to. + */ + if (n_socks > get_options()->ConnLimit_high_thresh) { + /* Try to get down to the low threshold */ + target_n_socks = get_options()->ConnLimit_low_thresh; + log_notice(LD_NET, + "Current number of sockets %d is greater than configured " + "limit %d; OOS handler trying to get down to %d", + n_socks, get_options()->ConnLimit_high_thresh, + target_n_socks); + } else if (failed) { + /* + * If we're not at the limit but we hit a socket exhaustion error, try to + * drop some (but not as aggressively as ConnLimit_low_threshold, which is + * 3/4 of ConnLimit_) + */ + target_n_socks = (n_socks * 9) / 10; + log_notice(LD_NET, + "We saw socket exhaustion at %d open sockets; OOS handler " + "trying to get down to %d", + n_socks, target_n_socks); + } + + if (target_n_socks > 0) { + /* + * It's an OOS! + * + * TODO count moribund sockets; it'll be important that anything we decide + * to get rid of here but don't immediately close get counted as moribund + * on subsequent invocations so we don't try to kill too many things if + * this gets called multiple times. + */ + + /* TODO pick what to try to close */ + } +} + /** Log how many bytes are used by buffers of different kinds and sizes. */ void connection_dump_buffer_mem_stats(int severity) diff --git a/src/or/connection.h b/src/or/connection.h index f8e0f73246..4e8a740601 100644 --- a/src/or/connection.h +++ b/src/or/connection.h @@ -247,6 +247,8 @@ void clock_skew_warning(const connection_t *conn, long apparent_skew, int trusted, log_domain_mask_t domain, const char *received, const char *source); +void connection_handle_oos(int n_socks, int failed); + #ifdef CONNECTION_PRIVATE STATIC void connection_free_(connection_t *conn); From 4f253d4c676caba979ddf4bc2668273700f279ae Mon Sep 17 00:00:00 2001 From: Andrea Shepard Date: Thu, 30 Jun 2016 10:58:17 +0000 Subject: [PATCH 03/17] Don't say OOS unless the ConnLimit thresholds have been set --- src/or/connection.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/or/connection.c b/src/or/connection.c index 3939531876..77ad56775c 100644 --- a/src/or/connection.c +++ b/src/or/connection.c @@ -4523,9 +4523,13 @@ connection_handle_oos(int n_socks, int failed) /* * Check if we're really handling an OOS condition, and if so decide how - * many sockets we want to get down to. + * many sockets we want to get down to. Be sure we check if the threshold + * is distinct from zero first; it's possible for this to be called a few + * times before we've finished reading the config. */ - if (n_socks > get_options()->ConnLimit_high_thresh) { + if (n_socks >= get_options()->ConnLimit_high_thresh && + get_options()->ConnLimit_high_thresh != 0 && + get_options()->ConnLimit_ != 0) { /* Try to get down to the low threshold */ target_n_socks = get_options()->ConnLimit_low_thresh; log_notice(LD_NET, From 2bc19171ef6d3d569b3116ce49e93b20bd9b7449 Mon Sep 17 00:00:00 2001 From: Andrea Shepard Date: Thu, 30 Jun 2016 13:27:35 +0000 Subject: [PATCH 04/17] Implement connection_count_moribund() for OOS handler --- src/or/connection.c | 20 ++++++++++++++++---- src/or/main.c | 22 ++++++++++++++++++++++ src/or/main.h | 2 ++ 3 files changed, 40 insertions(+), 4 deletions(-) diff --git a/src/or/connection.c b/src/or/connection.c index 77ad56775c..6ef20b2371 100644 --- a/src/or/connection.c +++ b/src/or/connection.c @@ -4508,7 +4508,7 @@ connection_reached_eof(connection_t *conn) void connection_handle_oos(int n_socks, int failed) { - int target_n_socks = 0; + int target_n_socks = 0, moribund_socks, socks_to_kill; /* Sanity-check args */ tor_assert(n_socks >= 0); @@ -4554,13 +4554,25 @@ connection_handle_oos(int n_socks, int failed) /* * It's an OOS! * - * TODO count moribund sockets; it'll be important that anything we decide + * Count moribund sockets; it's be important that anything we decide * to get rid of here but don't immediately close get counted as moribund * on subsequent invocations so we don't try to kill too many things if - * this gets called multiple times. + * connection_handle_oos() gets called multiple times. */ + moribund_socks = connection_count_moribund(); - /* TODO pick what to try to close */ + if (moribund_socks < n_socks - target_n_socks) { + socks_to_kill = n_socks - target_n_socks - moribund_socks; + /* TODO actually kill them */ + log_notice(LD_NET, + "OOS handler wants to kill %d sockets", + socks_to_kill); + } else { + log_notice(LD_NET, + "Not killing any sockets for OOS because there are %d " + "already moribund, and we only want to eliminate %d", + moribund_socks, n_socks - target_n_socks); + } } } diff --git a/src/or/main.c b/src/or/main.c index 261118a81e..30041c5e2b 100644 --- a/src/or/main.c +++ b/src/or/main.c @@ -651,6 +651,28 @@ close_closeable_connections(void) } } +/** Count moribund connections for the OOS handler */ +int +connection_count_moribund(void) +{ + int i, moribund = 0; + connection_t *conn; + + /* + * Count things we'll try to kill when close_closeable_connections() + * runs next. + */ + for (i = 0; i < smartlist_len(closeable_connection_lst); ++i) { + conn = smartlist_get(closeable_connection_lst, i); + if (conn->conn_array_index < 0 || + conn->marked_for_close) { + ++moribund; + } + } + + return moribund; +} + /** Libevent callback: this gets invoked when (connection_t*)conn has * some data to read. */ static void diff --git a/src/or/main.h b/src/or/main.h index 31a22de424..4f58f170d5 100644 --- a/src/or/main.h +++ b/src/or/main.h @@ -47,6 +47,8 @@ MOCK_DECL(void,connection_start_writing,(connection_t *conn)); void connection_stop_reading_from_linked_conn(connection_t *conn); +int connection_count_moribund(void); + void directory_all_unreachable(time_t now); void directory_info_has_arrived(time_t now, int from_cache, int suppress_logs); From c76d45bdecb0ac5f11b578919e491957bee41d8f Mon Sep 17 00:00:00 2001 From: Andrea Shepard Date: Thu, 30 Jun 2016 13:57:27 +0000 Subject: [PATCH 05/17] Stub out pick_oos_victims() and kill_conn_list_for_oos() --- src/or/connection.c | 39 +++++++++++++++++++++++++++++++++++---- 1 file changed, 35 insertions(+), 4 deletions(-) diff --git a/src/or/connection.c b/src/or/connection.c index 6ef20b2371..fce63adcbe 100644 --- a/src/or/connection.c +++ b/src/or/connection.c @@ -4500,6 +4500,30 @@ connection_reached_eof(connection_t *conn) } } +/** Pick n victim connections for the OOS handler and return them in a + * smartlist. + */ +static smartlist_t * +pick_oos_victims(int n) +{ + smartlist_t *conns = NULL; + + (void)n; + + /* TODO */ + + return conns; +} + +/** Kill a list of connections for the OOS handler. */ +static void +kill_conn_list_for_oos(smartlist_t *conns) +{ + (void)conns; + + /* TODO */ +} + /** Out-of-Sockets handler; n_socks is the current number of open * sockets, and failed is non-zero if a socket exhaustion related * error immediately preceded this call. This is where to do @@ -4509,6 +4533,7 @@ void connection_handle_oos(int n_socks, int failed) { int target_n_socks = 0, moribund_socks, socks_to_kill; + smartlist_t *conns; /* Sanity-check args */ tor_assert(n_socks >= 0); @@ -4563,10 +4588,16 @@ connection_handle_oos(int n_socks, int failed) if (moribund_socks < n_socks - target_n_socks) { socks_to_kill = n_socks - target_n_socks - moribund_socks; - /* TODO actually kill them */ - log_notice(LD_NET, - "OOS handler wants to kill %d sockets", - socks_to_kill); + + conns = pick_oos_victims(socks_to_kill); + if (conns) { + kill_conn_list_for_oos(conns); + log_notice(LD_NET, + "OOS handler killed %d conns", smartlist_len(conns)); + smartlist_free(conns); + } else { + log_notice(LD_NET, "OOS handler failed to pick any victim conns"); + } } else { log_notice(LD_NET, "Not killing any sockets for OOS because there are %d " From e9464737af4b31e0aba29eb5581a5faf80647800 Mon Sep 17 00:00:00 2001 From: Andrea Shepard Date: Thu, 30 Jun 2016 14:13:42 +0000 Subject: [PATCH 06/17] Implement kill_conn_list_for_oos() --- src/or/connection.c | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/src/or/connection.c b/src/or/connection.c index fce63adcbe..e164fae15c 100644 --- a/src/or/connection.c +++ b/src/or/connection.c @@ -4519,9 +4519,20 @@ pick_oos_victims(int n) static void kill_conn_list_for_oos(smartlist_t *conns) { - (void)conns; + if (!conns) return; - /* TODO */ + SMARTLIST_FOREACH_BEGIN(conns, connection_t *, c) { + /* Make sure the channel layer gets told about orconns */ + if (c->type == CONN_TYPE_OR) { + connection_or_close_for_error(TO_OR_CONN(c), 1); + } else { + connection_mark_and_flush(c); + } + } SMARTLIST_FOREACH_END(c); + + log_notice(LD_NET, + "OOS handler marked and flushed %d connections", + smartlist_len(conns)); } /** Out-of-Sockets handler; n_socks is the current number of open From d502efbd47abebea4f92ea55c4566f130c90ce0e Mon Sep 17 00:00:00 2001 From: Andrea Shepard Date: Thu, 30 Jun 2016 14:54:58 +0000 Subject: [PATCH 07/17] Implement pick_oos_victims() except for sort step --- src/or/connection.c | 83 ++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 79 insertions(+), 4 deletions(-) diff --git a/src/or/connection.c b/src/or/connection.c index e164fae15c..18b8a7e62e 100644 --- a/src/or/connection.c +++ b/src/or/connection.c @@ -4506,13 +4506,88 @@ connection_reached_eof(connection_t *conn) static smartlist_t * pick_oos_victims(int n) { - smartlist_t *conns = NULL; + smartlist_t *eligible = NULL, *victims = NULL; + smartlist_t *conns; + int conn_counts_by_type[CONN_TYPE_MAX_ + 1], i; - (void)n; + /* + * Big damn assumption (someone improve this someday!): + * + * Socket exhaustion normally happens on high-volume relays, and so + * most of the connections involved are orconns. We should pick victims + * by assembling a list of all orconns, and sorting them in order of + * how much 'damage' by some metric we'd be doing by dropping them. + * + * If we move on from orconns, we should probably think about incoming + * directory connections next, or exit connections. Things we should + * probably never kill are controller connections and listeners. + * + * This function will count how many connections of different types + * exist and log it for purposes of gathering data on typical OOS + * situations to guide future improvements. + */ - /* TODO */ + /* First, get the connection array */ + conns = get_connection_array(); + /* + * Iterate it and pick out eligible connection types, and log some stats + * along the way. + */ + eligible = smartlist_new(); + memset(conn_counts_by_type, 0, sizeof(conn_counts_by_type)); + SMARTLIST_FOREACH_BEGIN(conns, connection_t *, c) { + /* Bump the counter */ + tor_assert(c->type <= CONN_TYPE_MAX_); + ++(conn_counts_by_type[c->type]); - return conns; + /* Skip anything we would count as moribund */ + if (c->conn_array_index < 0 || + c->marked_for_close) { + continue; + } + + switch (c->type) { + case CONN_TYPE_OR: + /* We've got an orconn, it's eligible to be OOSed */ + smartlist_add(eligible, c); + break; + default: + /* We don't know what to do with it, ignore it */ + break; + } + } SMARTLIST_FOREACH_END(c); + + /* Log some stats */ + if (smartlist_len(conns) > 0) { + /* At least one counter must be non-zero */ + log_info(LD_NET, "Some stats on conn types seen during OOS follow"); + for (i = CONN_TYPE_MIN_; i <= CONN_TYPE_MAX_; ++i) { + /* Did we see any? */ + if (conn_counts_by_type[i] > 0) { + log_info(LD_NET, "%s: %d conns", + conn_type_to_string(i), + conn_counts_by_type[i]); + } + } + log_info(LD_NET, "Done with OOS conn type stats"); + } + + /* Did we find more eligible targets than we want to kill? */ + if (smartlist_len(eligible) > n) { + /* TODO sort */ + /* Pick first n as victims */ + victims = smartlist_new(); + for (i = 0; i < n; ++i) { + smartlist_add(victims, smartlist_get(eligible, i)); + } + /* Free the original list */ + smartlist_free(eligible); + } else { + /* No, we can just call them all victims */ + victims = eligible; + } + + return victims; } /** Kill a list of connections for the OOS handler. */ From 709f2cbf58c0acc4c84035dadc6be4846b5d3f3e Mon Sep 17 00:00:00 2001 From: Andrea Shepard Date: Fri, 1 Jul 2016 21:05:13 +0000 Subject: [PATCH 08/17] Implement OOS comparator --- src/or/connection.c | 50 ++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 49 insertions(+), 1 deletion(-) diff --git a/src/or/connection.c b/src/or/connection.c index 18b8a7e62e..9c80d97e4d 100644 --- a/src/or/connection.c +++ b/src/or/connection.c @@ -4500,6 +4500,53 @@ connection_reached_eof(connection_t *conn) } } +/** Comparator for the two-orconn case in OOS victim sort */ +static int +oos_victim_comparator_for_orconns(or_connection_t *a, or_connection_t *b) +{ + int a_circs, b_circs; + /* Fewer circuits == higher priority for OOS kill, sort earlier */ + + a_circs = connection_or_get_num_circuits(a); + b_circs = connection_or_get_num_circuits(b); + + if (a_circs < b_circs) return -1; + else if (b_circs > a_circs) return 1; + else return 0; +} + +/** Sort comparator for OOS victims; better targets sort before worse + * ones. */ +static int +oos_victim_comparator(const void **a_v, const void **b_v) +{ + connection_t *a = NULL, *b = NULL; + + /* Get connection pointers out */ + + a = (connection_t *)(*a_v); + b = (connection_t *)(*b_v); + + tor_assert(a != NULL); + tor_assert(b != NULL); + + /* + * We always prefer orconns as victims currently; we won't even see + * these non-orconn cases, but if we do, sort them after orconns. + */ + if (a->type == CONN_TYPE_OR && b->type == CONN_TYPE_OR) { + return oos_victim_comparator_for_orconns(TO_OR_CONN(a), TO_OR_CONN(b)); + } else { + /* + * One isn't an orconn; if one is, it goes first. We currently have no + * opinions about cases where neither is an orconn. + */ + if (a->type == CONN_TYPE_OR) return -1; + else if (b->type == CONN_TYPE_OR) return 1; + else return 0; + } +} + /** Pick n victim connections for the OOS handler and return them in a * smartlist. */ @@ -4574,7 +4621,8 @@ pick_oos_victims(int n) /* Did we find more eligible targets than we want to kill? */ if (smartlist_len(eligible) > n) { - /* TODO sort */ + /* Sort the list in order of target preference */ + smartlist_sort(eligible, oos_victim_comparator); /* Pick first n as victims */ victims = smartlist_new(); for (i = 0; i < n; ++i) { From 26c2ded00cfcf1c86ebe10a65b6de0af0e5b76c7 Mon Sep 17 00:00:00 2001 From: Andrea Shepard Date: Sat, 2 Jul 2016 04:39:45 +0000 Subject: [PATCH 09/17] Unit test for connection_handle_oos() --- src/or/connection.c | 8 +- src/or/connection.h | 3 + src/or/main.c | 4 +- src/or/main.h | 2 +- src/test/include.am | 1 + src/test/test.c | 1 + src/test/test.h | 1 + src/test/test_oos.c | 248 ++++++++++++++++++++++++++++++++++++++++++++ 8 files changed, 261 insertions(+), 7 deletions(-) create mode 100644 src/test/test_oos.c diff --git a/src/or/connection.c b/src/or/connection.c index 9c80d97e4d..844ab40b6e 100644 --- a/src/or/connection.c +++ b/src/or/connection.c @@ -4550,8 +4550,8 @@ oos_victim_comparator(const void **a_v, const void **b_v) /** Pick n victim connections for the OOS handler and return them in a * smartlist. */ -static smartlist_t * -pick_oos_victims(int n) +MOCK_IMPL(STATIC smartlist_t *, +pick_oos_victims, (int n)) { smartlist_t *eligible = NULL, *victims = NULL; smartlist_t *conns; @@ -4639,8 +4639,8 @@ pick_oos_victims(int n) } /** Kill a list of connections for the OOS handler. */ -static void -kill_conn_list_for_oos(smartlist_t *conns) +MOCK_IMPL(STATIC void, +kill_conn_list_for_oos, (smartlist_t *conns)) { if (!conns) return; diff --git a/src/or/connection.h b/src/or/connection.h index 4e8a740601..83ee76932b 100644 --- a/src/or/connection.h +++ b/src/or/connection.h @@ -267,6 +267,9 @@ MOCK_DECL(STATIC int,connection_connect_sockaddr, const struct sockaddr *bindaddr, socklen_t bindaddr_len, int *socket_error)); +MOCK_DECL(STATIC void, kill_conn_list_for_oos, (smartlist_t *conns)); +MOCK_DECL(STATIC smartlist_t *, pick_oos_victims, (int n)); + #endif #endif diff --git a/src/or/main.c b/src/or/main.c index 30041c5e2b..7f00cc3455 100644 --- a/src/or/main.c +++ b/src/or/main.c @@ -652,8 +652,8 @@ close_closeable_connections(void) } /** Count moribund connections for the OOS handler */ -int -connection_count_moribund(void) +MOCK_IMPL(int, +connection_count_moribund, (void)) { int i, moribund = 0; connection_t *conn; diff --git a/src/or/main.h b/src/or/main.h index 4f58f170d5..45c3b3ff78 100644 --- a/src/or/main.h +++ b/src/or/main.h @@ -47,7 +47,7 @@ MOCK_DECL(void,connection_start_writing,(connection_t *conn)); void connection_stop_reading_from_linked_conn(connection_t *conn); -int connection_count_moribund(void); +MOCK_DECL(int, connection_count_moribund, (void)); void directory_all_unreachable(time_t now); void directory_info_has_arrived(time_t now, int from_cache, int suppress_logs); diff --git a/src/test/include.am b/src/test/include.am index d0bc808877..2e91f7c9dc 100644 --- a/src/test/include.am +++ b/src/test/include.am @@ -103,6 +103,7 @@ src_test_test_SOURCES = \ src/test/test_microdesc.c \ src/test/test_nodelist.c \ src/test/test_oom.c \ + src/test/test_oos.c \ src/test/test_options.c \ src/test/test_policy.c \ src/test/test_procmon.c \ diff --git a/src/test/test.c b/src/test/test.c index f8610168f6..2f10c7e90b 100644 --- a/src/test/test.c +++ b/src/test/test.c @@ -1210,6 +1210,7 @@ struct testgroup_t testgroups[] = { { "link-handshake/", link_handshake_tests }, { "nodelist/", nodelist_tests }, { "oom/", oom_tests }, + { "oos/", oos_tests }, { "options/", options_tests }, { "policy/" , policy_tests }, { "procmon/", procmon_tests }, diff --git a/src/test/test.h b/src/test/test.h index 6744d255f1..c0643e154d 100644 --- a/src/test/test.h +++ b/src/test/test.h @@ -202,6 +202,7 @@ extern struct testcase_t logging_tests[]; extern struct testcase_t microdesc_tests[]; extern struct testcase_t nodelist_tests[]; extern struct testcase_t oom_tests[]; +extern struct testcase_t oos_tests[]; extern struct testcase_t options_tests[]; extern struct testcase_t policy_tests[]; extern struct testcase_t procmon_tests[]; diff --git a/src/test/test_oos.c b/src/test/test_oos.c new file mode 100644 index 0000000000..48e66fda53 --- /dev/null +++ b/src/test/test_oos.c @@ -0,0 +1,248 @@ +/* Copyright (c) 2016, The Tor Project, Inc. */ +/* See LICENSE for licensing information */ + +/* Unit tests for OOS handler */ + +#define CONNECTION_PRIVATE + +#include "or.h" +#include "config.h" +#include "connection.h" +#include "main.h" +#include "test.h" + +static or_options_t mock_options; + +static void +reset_options_mock(void) +{ + memset(&mock_options, 0, sizeof(or_options_t)); +} + +static const or_options_t * +mock_get_options(void) +{ + return &mock_options; +} + +static int moribund_calls = 0; +static int moribund_conns = 0; + +static int +mock_connection_count_moribund(void) +{ + ++moribund_calls; + + return moribund_conns; +} + +/* + * For unit test purposes it's sufficient to tell that + * kill_conn_list_for_oos() was called with an approximately + * sane argument; it's just the thing we returned from the + * mock for pick_oos_victims(). + */ + +static int kill_conn_list_calls = 0; +static int kill_conn_list_killed = 0; + +static void +kill_conn_list_mock(smartlist_t *conns) +{ + ++kill_conn_list_calls; + + tt_assert(conns != NULL); + + kill_conn_list_killed += smartlist_len(conns); + + done: + return; +} + +static int pick_oos_mock_calls = 0; +static int pick_oos_mock_fail = 0; +static int pick_oos_mock_last_n = 0; + +static smartlist_t * +pick_oos_victims_mock(int n) +{ + smartlist_t *l; + int i; + + ++pick_oos_mock_calls; + + tt_int_op(n, OP_GT, 0); + + if (!pick_oos_mock_fail) { + /* + * connection_handle_oos() just passes the list onto + * kill_conn_list_for_oos(); we don't need to simulate + * its content for this mock, just its existence, but + * we do need to check the parameter. + */ + l = smartlist_new(); + for (i = 0; i < n; ++i) smartlist_add(l, NULL); + } else { + l = NULL; + } + + pick_oos_mock_last_n = n; + + done: + return l; +} + +/** Unit test for the logic in connection_handle_oos(), which is concerned + * with comparing thresholds and connection counts to decide if an OOS has + * occurred and if so, how many connections to try to kill, and then using + * pick_oos_victims() and kill_conn_list_for_oos() to carry out its grim + * duty. + */ +static void +test_oos_connection_handle_oos(void *arg) +{ + (void)arg; + + /* Set up mocks */ + reset_options_mock(); + /* OOS handling is only sensitive to these fields */ + mock_options.ConnLimit = 32; + mock_options.ConnLimit_ = 64; + mock_options.ConnLimit_high_thresh = 60; + mock_options.ConnLimit_low_thresh = 50; + MOCK(get_options, mock_get_options); + moribund_calls = 0; + moribund_conns = 0; + MOCK(connection_count_moribund, mock_connection_count_moribund); + kill_conn_list_calls = 0; + kill_conn_list_killed = 0; + MOCK(kill_conn_list_for_oos, kill_conn_list_mock); + pick_oos_mock_calls = 0; + pick_oos_mock_fail = 0; + MOCK(pick_oos_victims, pick_oos_victims_mock); + + /* No OOS case */ + connection_handle_oos(50, 0); + tt_int_op(moribund_calls, OP_EQ, 0); + tt_int_op(pick_oos_mock_calls, OP_EQ, 0); + tt_int_op(kill_conn_list_calls, OP_EQ, 0); + + /* OOS from socket count, nothing moribund */ + connection_handle_oos(62, 0); + tt_int_op(moribund_calls, OP_EQ, 1); + tt_int_op(pick_oos_mock_calls, OP_EQ, 1); + /* 12 == 62 - ConnLimit_low_thresh */ + tt_int_op(pick_oos_mock_last_n, OP_EQ, 12); + tt_int_op(kill_conn_list_calls, OP_EQ, 1); + tt_int_op(kill_conn_list_killed, OP_EQ, 12); + + /* OOS from socket count, some are moribund */ + kill_conn_list_killed = 0; + moribund_conns = 5; + connection_handle_oos(62, 0); + tt_int_op(moribund_calls, OP_EQ, 2); + tt_int_op(pick_oos_mock_calls, OP_EQ, 2); + /* 7 == 62 - ConnLimit_low_thresh - moribund_conns */ + tt_int_op(pick_oos_mock_last_n, OP_EQ, 7); + tt_int_op(kill_conn_list_calls, OP_EQ, 2); + tt_int_op(kill_conn_list_killed, OP_EQ, 7); + + /* OOS from socket count, but pick fails */ + kill_conn_list_killed = 0; + moribund_conns = 0; + pick_oos_mock_fail = 1; + connection_handle_oos(62, 0); + tt_int_op(moribund_calls, OP_EQ, 3); + tt_int_op(pick_oos_mock_calls, OP_EQ, 3); + tt_int_op(kill_conn_list_calls, OP_EQ, 2); + tt_int_op(kill_conn_list_killed, OP_EQ, 0); + pick_oos_mock_fail = 0; + + /* + * OOS from socket count with so many moribund conns + * we have none to kill. + */ + kill_conn_list_killed = 0; + moribund_conns = 15; + connection_handle_oos(62, 0); + tt_int_op(moribund_calls, OP_EQ, 4); + tt_int_op(pick_oos_mock_calls, OP_EQ, 3); + tt_int_op(kill_conn_list_calls, OP_EQ, 2); + + /* + * OOS from socket exhaustion; OOS handler will try to + * kill 1/10 (5) of the connections. + */ + kill_conn_list_killed = 0; + moribund_conns = 0; + connection_handle_oos(50, 1); + tt_int_op(moribund_calls, OP_EQ, 5); + tt_int_op(pick_oos_mock_calls, OP_EQ, 4); + tt_int_op(kill_conn_list_calls, OP_EQ, 3); + tt_int_op(kill_conn_list_killed, OP_EQ, 5); + + /* OOS from socket exhaustion with moribund conns */ + kill_conn_list_killed = 0; + moribund_conns = 2; + connection_handle_oos(50, 1); + tt_int_op(moribund_calls, OP_EQ, 6); + tt_int_op(pick_oos_mock_calls, OP_EQ, 5); + tt_int_op(kill_conn_list_calls, OP_EQ, 4); + tt_int_op(kill_conn_list_killed, OP_EQ, 3); + + /* OOS from socket exhaustion with many moribund conns */ + kill_conn_list_killed = 0; + moribund_conns = 7; + connection_handle_oos(50, 1); + tt_int_op(moribund_calls, OP_EQ, 7); + tt_int_op(pick_oos_mock_calls, OP_EQ, 5); + tt_int_op(kill_conn_list_calls, OP_EQ, 4); + + /* OOS with both socket exhaustion and above-threshold */ + kill_conn_list_killed = 0; + moribund_conns = 0; + connection_handle_oos(62, 1); + tt_int_op(moribund_calls, OP_EQ, 8); + tt_int_op(pick_oos_mock_calls, OP_EQ, 6); + tt_int_op(kill_conn_list_calls, OP_EQ, 5); + tt_int_op(kill_conn_list_killed, OP_EQ, 12); + + /* + * OOS with both socket exhaustion and above-threshold with some + * moribund conns + */ + kill_conn_list_killed = 0; + moribund_conns = 5; + connection_handle_oos(62, 1); + tt_int_op(moribund_calls, OP_EQ, 9); + tt_int_op(pick_oos_mock_calls, OP_EQ, 7); + tt_int_op(kill_conn_list_calls, OP_EQ, 6); + tt_int_op(kill_conn_list_killed, OP_EQ, 7); + + /* + * OOS with both socket exhaustion and above-threshold with many + * moribund conns + */ + kill_conn_list_killed = 0; + moribund_conns = 15; + connection_handle_oos(62, 1); + tt_int_op(moribund_calls, OP_EQ, 10); + tt_int_op(pick_oos_mock_calls, OP_EQ, 7); + tt_int_op(kill_conn_list_calls, OP_EQ, 6); + + done: + + UNMOCK(pick_oos_victims); + UNMOCK(kill_conn_list_for_oos); + UNMOCK(connection_count_moribund); + UNMOCK(get_options); + + return; +} + +struct testcase_t oos_tests[] = { + { "connection_handle_oos", test_oos_connection_handle_oos, + TT_FORK, NULL, NULL }, + END_OF_TESTCASES +}; + From e17083b4328490d8785c798eb4f41f653e6548d7 Mon Sep 17 00:00:00 2001 From: Andrea Shepard Date: Sat, 2 Jul 2016 05:42:09 +0000 Subject: [PATCH 10/17] Unit test for kill_conn_list_for_oos() --- src/or/connection.c | 6 ++-- src/or/connection.h | 4 +-- src/test/test_oos.c | 84 +++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 89 insertions(+), 5 deletions(-) diff --git a/src/or/connection.c b/src/or/connection.c index 844ab40b6e..09b4733234 100644 --- a/src/or/connection.c +++ b/src/or/connection.c @@ -754,9 +754,9 @@ connection_mark_for_close_(connection_t *conn, int line, const char *file) * For all other cases, use connection_mark_and_flush() instead, which * checks for or_connection_t properly, instead. See below. */ -void -connection_mark_for_close_internal_(connection_t *conn, - int line, const char *file) +MOCK_IMPL(void, +connection_mark_for_close_internal_, (connection_t *conn, + int line, const char *file)) { assert_connection_ok(conn,0); tor_assert(line); diff --git a/src/or/connection.h b/src/or/connection.h index 83ee76932b..e2f1764549 100644 --- a/src/or/connection.h +++ b/src/or/connection.h @@ -34,8 +34,8 @@ void connection_about_to_close_connection(connection_t *conn); void connection_close_immediate(connection_t *conn); void connection_mark_for_close_(connection_t *conn, int line, const char *file); -void connection_mark_for_close_internal_(connection_t *conn, - int line, const char *file); +MOCK_DECL(void, connection_mark_for_close_internal_, + (connection_t *conn, int line, const char *file)); #define connection_mark_for_close(c) \ connection_mark_for_close_((c), __LINE__, SHORT_FILE__) diff --git a/src/test/test_oos.c b/src/test/test_oos.c index 48e66fda53..0f904f65b8 100644 --- a/src/test/test_oos.c +++ b/src/test/test_oos.c @@ -8,6 +8,7 @@ #include "or.h" #include "config.h" #include "connection.h" +#include "connection_or.h" #include "main.h" #include "test.h" @@ -240,9 +241,92 @@ test_oos_connection_handle_oos(void *arg) return; } +static int cfe_calls = 0; + +static void +close_for_error_mock(or_connection_t *orconn, int flush) +{ + (void)flush; + + tt_assert(orconn != NULL); + ++cfe_calls; + + done: + return; +} + +static int mark_calls = 0; + +static void +mark_for_close_oos_mock(connection_t *conn, + int line, const char *file) +{ + (void)line; + (void)file; + + tt_assert(conn != NULL); + ++mark_calls; + + done: + return; +} + +static void +test_oos_kill_conn_list(void *arg) +{ + connection_t *c1, *c2; + or_connection_t *or_c1 = NULL; + dir_connection_t *dir_c2 = NULL; + smartlist_t *l = NULL; + (void)arg; + + /* Set up mocks */ + mark_calls = 0; + MOCK(connection_mark_for_close_internal_, mark_for_close_oos_mock); + cfe_calls = 0; + MOCK(connection_or_close_for_error, close_for_error_mock); + + /* Make fake conns */ + or_c1 = tor_malloc_zero(sizeof(*or_c1)); + or_c1->base_.magic = OR_CONNECTION_MAGIC; + or_c1->base_.type = CONN_TYPE_OR; + c1 = TO_CONN(or_c1); + dir_c2 = tor_malloc_zero(sizeof(*dir_c2)); + dir_c2->base_.magic = DIR_CONNECTION_MAGIC; + dir_c2->base_.type = CONN_TYPE_DIR; + c2 = TO_CONN(dir_c2); + + tt_assert(c1 != NULL); + tt_assert(c2 != NULL); + + /* Make list */ + l = smartlist_new(); + smartlist_add(l, c1); + smartlist_add(l, c2); + + /* Run kill_conn_list_for_oos() */ + kill_conn_list_for_oos(l); + + /* Check call counters */ + tt_int_op(mark_calls, OP_EQ, 1); + tt_int_op(cfe_calls, OP_EQ, 1); + + done: + + UNMOCK(connection_or_close_for_error); + UNMOCK(connection_mark_for_close_internal_); + + if (l) smartlist_free(l); + tor_free(or_c1); + tor_free(dir_c2); + + return; +} + struct testcase_t oos_tests[] = { { "connection_handle_oos", test_oos_connection_handle_oos, TT_FORK, NULL, NULL }, + { "kill_conn_list", test_oos_kill_conn_list, TT_FORK, NULL, NULL }, END_OF_TESTCASES }; From d65f03091554e94ec1ceb51a607df19e9943c023 Mon Sep 17 00:00:00 2001 From: Andrea Shepard Date: Sat, 2 Jul 2016 08:02:47 +0000 Subject: [PATCH 11/17] Unit test for pick_oos_victims() --- src/or/connection_or.c | 4 +- src/or/connection_or.h | 2 +- src/or/main.c | 4 +- src/or/main.h | 2 +- src/test/test_oos.c | 124 ++++++++++++++++++++++++++++++++++++++++- 5 files changed, 129 insertions(+), 7 deletions(-) diff --git a/src/or/connection_or.c b/src/or/connection_or.c index 1f0c4bdef5..72d8e13e90 100644 --- a/src/or/connection_or.c +++ b/src/or/connection_or.c @@ -394,8 +394,8 @@ connection_or_change_state(or_connection_t *conn, uint8_t state) * be an or_connection_t field, but it got moved to channel_t and we * shouldn't maintain two copies. */ -int -connection_or_get_num_circuits(or_connection_t *conn) +MOCK_IMPL(int, +connection_or_get_num_circuits, (or_connection_t *conn)) { tor_assert(conn); diff --git a/src/or/connection_or.h b/src/or/connection_or.h index e2ec47a4f2..2e8c6066cc 100644 --- a/src/or/connection_or.h +++ b/src/or/connection_or.h @@ -64,7 +64,7 @@ void connection_or_init_conn_from_address(or_connection_t *conn, int connection_or_client_learned_peer_id(or_connection_t *conn, const uint8_t *peer_id); time_t connection_or_client_used(or_connection_t *conn); -int connection_or_get_num_circuits(or_connection_t *conn); +MOCK_DECL(int, connection_or_get_num_circuits, (or_connection_t *conn)); void or_handshake_state_free(or_handshake_state_t *state); void or_handshake_state_record_cell(or_connection_t *conn, or_handshake_state_t *state, diff --git a/src/or/main.c b/src/or/main.c index 7f00cc3455..18c09487c3 100644 --- a/src/or/main.c +++ b/src/or/main.c @@ -381,8 +381,8 @@ connection_in_array(connection_t *conn) /** Set *array to an array of all connections. *array must not * be modified. */ -smartlist_t * -get_connection_array(void) +MOCK_IMPL(smartlist_t *, +get_connection_array, (void)) { if (!connection_array) connection_array = smartlist_new(); diff --git a/src/or/main.h b/src/or/main.h index 45c3b3ff78..0220ae3c57 100644 --- a/src/or/main.h +++ b/src/or/main.h @@ -25,7 +25,7 @@ int connection_in_array(connection_t *conn); void add_connection_to_closeable_list(connection_t *conn); int connection_is_on_closeable_list(connection_t *conn); -smartlist_t *get_connection_array(void); +MOCK_DECL(smartlist_t *, get_connection_array, (void)); MOCK_DECL(uint64_t,get_bytes_read,(void)); MOCK_DECL(uint64_t,get_bytes_written,(void)); diff --git a/src/test/test_oos.c b/src/test/test_oos.c index 0f904f65b8..69a6fef09b 100644 --- a/src/test/test_oos.c +++ b/src/test/test_oos.c @@ -312,7 +312,7 @@ test_oos_kill_conn_list(void *arg) tt_int_op(cfe_calls, OP_EQ, 1); done: - + UNMOCK(connection_or_close_for_error); UNMOCK(connection_mark_for_close_internal_); @@ -323,10 +323,132 @@ test_oos_kill_conn_list(void *arg) return; } +static smartlist_t *conns_for_mock = NULL; + +static smartlist_t * +get_conns_mock(void) +{ + return conns_for_mock; +} + +/* + * For this mock, we pretend all conns have either zero or one circuits, + * depending on if this appears on the list of things to say have a circuit. + */ + +static smartlist_t *conns_with_circs = NULL; + +static int +get_num_circuits_mock(or_connection_t *conn) +{ + int circs = 0; + + tt_assert(conn != NULL); + + if (conns_with_circs && + smartlist_contains(conns_with_circs, TO_CONN(conn))) { + circs = 1; + } + + done: + return circs; +} + +static void +test_oos_pick_oos_victims(void *arg) +{ + (void)arg; + or_connection_t *ortmp; + dir_connection_t *dirtmp; + smartlist_t *picked; + + /* Set up mocks */ + conns_for_mock = smartlist_new(); + MOCK(get_connection_array, get_conns_mock); + conns_with_circs = smartlist_new(); + MOCK(connection_or_get_num_circuits, get_num_circuits_mock); + + /* Make some fake connections */ + ortmp = tor_malloc_zero(sizeof(*ortmp)); + ortmp->base_.magic = OR_CONNECTION_MAGIC; + ortmp->base_.type = CONN_TYPE_OR; + smartlist_add(conns_for_mock, TO_CONN(ortmp)); + /* We'll pretend this one has a circuit too */ + smartlist_add(conns_with_circs, TO_CONN(ortmp)); + /* Next one */ + ortmp = tor_malloc_zero(sizeof(*ortmp)); + ortmp->base_.magic = OR_CONNECTION_MAGIC; + ortmp->base_.type = CONN_TYPE_OR; + smartlist_add(conns_for_mock, TO_CONN(ortmp)); + /* Next one is moribund */ + ortmp = tor_malloc_zero(sizeof(*ortmp)); + ortmp->base_.magic = OR_CONNECTION_MAGIC; + ortmp->base_.type = CONN_TYPE_OR; + ortmp->base_.marked_for_close = 1; + smartlist_add(conns_for_mock, TO_CONN(ortmp)); + /* Last one isn't an orconn */ + dirtmp = tor_malloc_zero(sizeof(*dirtmp)); + dirtmp->base_.magic = DIR_CONNECTION_MAGIC; + dirtmp->base_.type = CONN_TYPE_DIR; + smartlist_add(conns_for_mock, TO_CONN(dirtmp)); + + /* Try picking one */ + picked = pick_oos_victims(1); + /* It should be the one with circuits */ + tt_assert(picked != NULL); + tt_int_op(smartlist_len(picked), OP_EQ, 1); + tt_assert(smartlist_contains(picked, smartlist_get(conns_for_mock, 0))); + smartlist_free(picked); + + /* Try picking none */ + picked = pick_oos_victims(0); + /* We should get an empty list */ + tt_assert(picked != NULL); + tt_int_op(smartlist_len(picked), OP_EQ, 0); + smartlist_free(picked); + + /* Try picking two */ + picked = pick_oos_victims(2); + /* We should get both active orconns */ + tt_assert(picked != NULL); + tt_int_op(smartlist_len(picked), OP_EQ, 2); + tt_assert(smartlist_contains(picked, smartlist_get(conns_for_mock, 0))); + tt_assert(smartlist_contains(picked, smartlist_get(conns_for_mock, 1))); + smartlist_free(picked); + + /* Try picking three - only two are eligible */ + picked = pick_oos_victims(3); + tt_int_op(smartlist_len(picked), OP_EQ, 2); + tt_assert(smartlist_contains(picked, smartlist_get(conns_for_mock, 0))); + tt_assert(smartlist_contains(picked, smartlist_get(conns_for_mock, 1))); + smartlist_free(picked); + + done: + + /* Free leftover stuff */ + if (conns_with_circs) { + smartlist_free(conns_with_circs); + conns_with_circs = NULL; + } + + UNMOCK(connection_or_get_num_circuits); + + if (conns_for_mock) { + SMARTLIST_FOREACH(conns_for_mock, connection_t *, c, tor_free(c)); + smartlist_free(conns_for_mock); + conns_for_mock = NULL; + } + + UNMOCK(get_connection_array); + + return; +} + struct testcase_t oos_tests[] = { { "connection_handle_oos", test_oos_connection_handle_oos, TT_FORK, NULL, NULL }, { "kill_conn_list", test_oos_kill_conn_list, TT_FORK, NULL, NULL }, + { "pick_oos_victims", test_oos_pick_oos_victims, TT_FORK, NULL, NULL }, END_OF_TESTCASES }; From dbdac1dc27b4ad1d98e8280e1f8b3a3aa2f60c15 Mon Sep 17 00:00:00 2001 From: Andrea Shepard Date: Sat, 20 Aug 2016 02:44:33 +0000 Subject: [PATCH 12/17] s/connection_handle_oos/connection_check_oos/g per code review --- src/or/config.c | 2 +- src/or/connection.c | 22 +++++++++++----------- src/or/connection.h | 2 +- src/test/test_oos.c | 30 +++++++++++++++--------------- 4 files changed, 28 insertions(+), 28 deletions(-) diff --git a/src/or/config.c b/src/or/config.c index f7a7a534b7..4831bad07a 100644 --- a/src/or/config.c +++ b/src/or/config.c @@ -1360,7 +1360,7 @@ options_act_reversible(const or_options_t *old_options, char **msg) options->ConnLimit_low_thresh); /* Give the OOS handler a chance with the new thresholds */ - connection_handle_oos(get_n_open_sockets(), 0); + connection_check_oos(get_n_open_sockets(), 0); } goto done; diff --git a/src/or/connection.c b/src/or/connection.c index 09b4733234..f38dc59a01 100644 --- a/src/or/connection.c +++ b/src/or/connection.c @@ -1359,7 +1359,7 @@ connection_listener_new(const struct sockaddr *listensockaddr, * Normal exit; call the OOS handler since connection count just changed; * the exhaustion flag will always be zero here though. */ - connection_handle_oos(get_n_open_sockets(), 0); + connection_check_oos(get_n_open_sockets(), 0); return conn; @@ -1370,7 +1370,7 @@ connection_listener_new(const struct sockaddr *listensockaddr, connection_free(conn); /* Call the OOS handler, indicate if we saw an exhaustion-related error */ - connection_handle_oos(get_n_open_sockets(), exhaustion); + connection_check_oos(get_n_open_sockets(), exhaustion); return NULL; } @@ -1467,12 +1467,12 @@ connection_handle_listener_read(connection_t *conn, int new_type) * * give the OOS handler a chance to run though */ - connection_handle_oos(get_n_open_sockets(), 0); + connection_check_oos(get_n_open_sockets(), 0); return 0; } else if (ERRNO_IS_RESOURCE_LIMIT(e)) { warn_too_many_conns(); /* Exhaustion; tell the OOS handler */ - connection_handle_oos(get_n_open_sockets(), 1); + connection_check_oos(get_n_open_sockets(), 1); return 0; } /* else there was a real error. */ @@ -1480,7 +1480,7 @@ connection_handle_listener_read(connection_t *conn, int new_type) tor_socket_strerror(e)); connection_mark_for_close(conn); /* Tell the OOS handler about this too */ - connection_handle_oos(get_n_open_sockets(), 0); + connection_check_oos(get_n_open_sockets(), 0); return -1; } log_debug(LD_NET, @@ -1488,7 +1488,7 @@ connection_handle_listener_read(connection_t *conn, int new_type) (int)news,(int)conn->s); /* We accepted a new conn; run OOS handler */ - connection_handle_oos(get_n_open_sockets(), 0); + connection_check_oos(get_n_open_sockets(), 0); if (make_socket_reuseable(news) < 0) { if (tor_socket_errno(news) == EINVAL) { @@ -1701,11 +1701,11 @@ connection_connect_sockaddr,(connection_t *conn, *socket_error = tor_socket_errno(s); if (ERRNO_IS_RESOURCE_LIMIT(*socket_error)) { warn_too_many_conns(); - connection_handle_oos(get_n_open_sockets(), 1); + connection_check_oos(get_n_open_sockets(), 1); } else { log_warn(LD_NET,"Error creating network socket: %s", tor_socket_strerror(*socket_error)); - connection_handle_oos(get_n_open_sockets(), 0); + connection_check_oos(get_n_open_sockets(), 0); } return -1; } @@ -1720,7 +1720,7 @@ connection_connect_sockaddr,(connection_t *conn, * against configuured maximum socket number, but tell it no exhaustion * failure. */ - connection_handle_oos(get_n_open_sockets(), 0); + connection_check_oos(get_n_open_sockets(), 0); if (bindaddr && bind(s, bindaddr, bindaddr_len) < 0) { *socket_error = tor_socket_errno(s); @@ -4664,7 +4664,7 @@ kill_conn_list_for_oos, (smartlist_t *conns)) * circuit-killing heuristics as needed. */ void -connection_handle_oos(int n_socks, int failed) +connection_check_oos(int n_socks, int failed) { int target_n_socks = 0, moribund_socks, socks_to_kill; smartlist_t *conns; @@ -4716,7 +4716,7 @@ connection_handle_oos(int n_socks, int failed) * Count moribund sockets; it's be important that anything we decide * to get rid of here but don't immediately close get counted as moribund * on subsequent invocations so we don't try to kill too many things if - * connection_handle_oos() gets called multiple times. + * connection_check_oos() gets called multiple times. */ moribund_socks = connection_count_moribund(); diff --git a/src/or/connection.h b/src/or/connection.h index e2f1764549..9bf063fbff 100644 --- a/src/or/connection.h +++ b/src/or/connection.h @@ -247,7 +247,7 @@ void clock_skew_warning(const connection_t *conn, long apparent_skew, int trusted, log_domain_mask_t domain, const char *received, const char *source); -void connection_handle_oos(int n_socks, int failed); +void connection_check_oos(int n_socks, int failed); #ifdef CONNECTION_PRIVATE STATIC void connection_free_(connection_t *conn); diff --git a/src/test/test_oos.c b/src/test/test_oos.c index 69a6fef09b..9451c5c537 100644 --- a/src/test/test_oos.c +++ b/src/test/test_oos.c @@ -76,7 +76,7 @@ pick_oos_victims_mock(int n) if (!pick_oos_mock_fail) { /* - * connection_handle_oos() just passes the list onto + * connection_check_oos() just passes the list onto * kill_conn_list_for_oos(); we don't need to simulate * its content for this mock, just its existence, but * we do need to check the parameter. @@ -93,14 +93,14 @@ pick_oos_victims_mock(int n) return l; } -/** Unit test for the logic in connection_handle_oos(), which is concerned +/** Unit test for the logic in connection_check_oos(), which is concerned * with comparing thresholds and connection counts to decide if an OOS has * occurred and if so, how many connections to try to kill, and then using * pick_oos_victims() and kill_conn_list_for_oos() to carry out its grim * duty. */ static void -test_oos_connection_handle_oos(void *arg) +test_oos_connection_check_oos(void *arg) { (void)arg; @@ -123,13 +123,13 @@ test_oos_connection_handle_oos(void *arg) MOCK(pick_oos_victims, pick_oos_victims_mock); /* No OOS case */ - connection_handle_oos(50, 0); + connection_check_oos(50, 0); tt_int_op(moribund_calls, OP_EQ, 0); tt_int_op(pick_oos_mock_calls, OP_EQ, 0); tt_int_op(kill_conn_list_calls, OP_EQ, 0); /* OOS from socket count, nothing moribund */ - connection_handle_oos(62, 0); + connection_check_oos(62, 0); tt_int_op(moribund_calls, OP_EQ, 1); tt_int_op(pick_oos_mock_calls, OP_EQ, 1); /* 12 == 62 - ConnLimit_low_thresh */ @@ -140,7 +140,7 @@ test_oos_connection_handle_oos(void *arg) /* OOS from socket count, some are moribund */ kill_conn_list_killed = 0; moribund_conns = 5; - connection_handle_oos(62, 0); + connection_check_oos(62, 0); tt_int_op(moribund_calls, OP_EQ, 2); tt_int_op(pick_oos_mock_calls, OP_EQ, 2); /* 7 == 62 - ConnLimit_low_thresh - moribund_conns */ @@ -152,7 +152,7 @@ test_oos_connection_handle_oos(void *arg) kill_conn_list_killed = 0; moribund_conns = 0; pick_oos_mock_fail = 1; - connection_handle_oos(62, 0); + connection_check_oos(62, 0); tt_int_op(moribund_calls, OP_EQ, 3); tt_int_op(pick_oos_mock_calls, OP_EQ, 3); tt_int_op(kill_conn_list_calls, OP_EQ, 2); @@ -165,7 +165,7 @@ test_oos_connection_handle_oos(void *arg) */ kill_conn_list_killed = 0; moribund_conns = 15; - connection_handle_oos(62, 0); + connection_check_oos(62, 0); tt_int_op(moribund_calls, OP_EQ, 4); tt_int_op(pick_oos_mock_calls, OP_EQ, 3); tt_int_op(kill_conn_list_calls, OP_EQ, 2); @@ -176,7 +176,7 @@ test_oos_connection_handle_oos(void *arg) */ kill_conn_list_killed = 0; moribund_conns = 0; - connection_handle_oos(50, 1); + connection_check_oos(50, 1); tt_int_op(moribund_calls, OP_EQ, 5); tt_int_op(pick_oos_mock_calls, OP_EQ, 4); tt_int_op(kill_conn_list_calls, OP_EQ, 3); @@ -185,7 +185,7 @@ test_oos_connection_handle_oos(void *arg) /* OOS from socket exhaustion with moribund conns */ kill_conn_list_killed = 0; moribund_conns = 2; - connection_handle_oos(50, 1); + connection_check_oos(50, 1); tt_int_op(moribund_calls, OP_EQ, 6); tt_int_op(pick_oos_mock_calls, OP_EQ, 5); tt_int_op(kill_conn_list_calls, OP_EQ, 4); @@ -194,7 +194,7 @@ test_oos_connection_handle_oos(void *arg) /* OOS from socket exhaustion with many moribund conns */ kill_conn_list_killed = 0; moribund_conns = 7; - connection_handle_oos(50, 1); + connection_check_oos(50, 1); tt_int_op(moribund_calls, OP_EQ, 7); tt_int_op(pick_oos_mock_calls, OP_EQ, 5); tt_int_op(kill_conn_list_calls, OP_EQ, 4); @@ -202,7 +202,7 @@ test_oos_connection_handle_oos(void *arg) /* OOS with both socket exhaustion and above-threshold */ kill_conn_list_killed = 0; moribund_conns = 0; - connection_handle_oos(62, 1); + connection_check_oos(62, 1); tt_int_op(moribund_calls, OP_EQ, 8); tt_int_op(pick_oos_mock_calls, OP_EQ, 6); tt_int_op(kill_conn_list_calls, OP_EQ, 5); @@ -214,7 +214,7 @@ test_oos_connection_handle_oos(void *arg) */ kill_conn_list_killed = 0; moribund_conns = 5; - connection_handle_oos(62, 1); + connection_check_oos(62, 1); tt_int_op(moribund_calls, OP_EQ, 9); tt_int_op(pick_oos_mock_calls, OP_EQ, 7); tt_int_op(kill_conn_list_calls, OP_EQ, 6); @@ -226,7 +226,7 @@ test_oos_connection_handle_oos(void *arg) */ kill_conn_list_killed = 0; moribund_conns = 15; - connection_handle_oos(62, 1); + connection_check_oos(62, 1); tt_int_op(moribund_calls, OP_EQ, 10); tt_int_op(pick_oos_mock_calls, OP_EQ, 7); tt_int_op(kill_conn_list_calls, OP_EQ, 6); @@ -445,7 +445,7 @@ test_oos_pick_oos_victims(void *arg) } struct testcase_t oos_tests[] = { - { "connection_handle_oos", test_oos_connection_handle_oos, + { "connection_check_oos", test_oos_connection_check_oos, TT_FORK, NULL, NULL }, { "kill_conn_list", test_oos_kill_conn_list, TT_FORK, NULL, NULL }, { "pick_oos_victims", test_oos_pick_oos_victims, TT_FORK, NULL, NULL }, From a403230fe38d396189a32bc525aa596444d88b61 Mon Sep 17 00:00:00 2001 From: Andrea Shepard Date: Sat, 20 Aug 2016 03:12:58 +0000 Subject: [PATCH 13/17] Use SMARTLIST_FOREACH in connection_count_moribund() per code review --- src/or/main.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/src/or/main.c b/src/or/main.c index 18c09487c3..9741f15022 100644 --- a/src/or/main.c +++ b/src/or/main.c @@ -655,20 +655,18 @@ close_closeable_connections(void) MOCK_IMPL(int, connection_count_moribund, (void)) { - int i, moribund = 0; - connection_t *conn; + int moribund = 0; /* * Count things we'll try to kill when close_closeable_connections() * runs next. */ - for (i = 0; i < smartlist_len(closeable_connection_lst); ++i) { - conn = smartlist_get(closeable_connection_lst, i); + SMARTLIST_FOREACH_BEGIN(closeable_connection_lst, connection_t *, conn) { if (conn->conn_array_index < 0 || conn->marked_for_close) { ++moribund; } - } + } SMARTLIST_FOREACH_END(conn); return moribund; } From 1a7709d40949b559a6f86e2bbcd1648eeeeed1af Mon Sep 17 00:00:00 2001 From: Andrea Shepard Date: Sat, 20 Aug 2016 03:34:16 +0000 Subject: [PATCH 14/17] Add connection_is_moribund() inline --- src/or/connection.c | 3 +-- src/or/connection.h | 14 ++++++++++++++ src/or/main.c | 5 +---- 3 files changed, 16 insertions(+), 6 deletions(-) diff --git a/src/or/connection.c b/src/or/connection.c index f38dc59a01..713a70cca4 100644 --- a/src/or/connection.c +++ b/src/or/connection.c @@ -4588,8 +4588,7 @@ pick_oos_victims, (int n)) ++(conn_counts_by_type[c->type]); /* Skip anything we would count as moribund */ - if (c->conn_array_index < 0 || - c->marked_for_close) { + if (connection_is_moribund(c)) { continue; } diff --git a/src/or/connection.h b/src/or/connection.h index 9bf063fbff..d25e002fa4 100644 --- a/src/or/connection.h +++ b/src/or/connection.h @@ -247,6 +247,20 @@ void clock_skew_warning(const connection_t *conn, long apparent_skew, int trusted, log_domain_mask_t domain, const char *received, const char *source); +/** Check if a connection is on the way out so the OOS handler doesn't try + * to kill more than it needs. */ +static inline int +connection_is_moribund(connection_t *conn) +{ + if (conn != NULL && + (conn->conn_array_index < 0 || + conn->marked_for_close)) { + return 1; + } else { + return 0; + } +} + void connection_check_oos(int n_socks, int failed); #ifdef CONNECTION_PRIVATE diff --git a/src/or/main.c b/src/or/main.c index 9741f15022..6ab94f5e36 100644 --- a/src/or/main.c +++ b/src/or/main.c @@ -662,10 +662,7 @@ connection_count_moribund, (void)) * runs next. */ SMARTLIST_FOREACH_BEGIN(closeable_connection_lst, connection_t *, conn) { - if (conn->conn_array_index < 0 || - conn->marked_for_close) { - ++moribund; - } + if (connection_is_moribund(conn)) ++moribund; } SMARTLIST_FOREACH_END(conn); return moribund; From 3efe92ba58137022dd6c61cff9823813218a1a14 Mon Sep 17 00:00:00 2001 From: Andrea Shepard Date: Sat, 20 Aug 2016 03:57:32 +0000 Subject: [PATCH 15/17] Consider the case that a connection doesn't have a valid socket during OOS --- src/or/connection.c | 5 +++++ src/or/main.c | 2 +- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/src/or/connection.c b/src/or/connection.c index 713a70cca4..6b85bfb550 100644 --- a/src/or/connection.c +++ b/src/or/connection.c @@ -4587,6 +4587,11 @@ pick_oos_victims, (int n)) tor_assert(c->type <= CONN_TYPE_MAX_); ++(conn_counts_by_type[c->type]); + /* Skip anything without a socket we can free */ + if (!(SOCKET_OK(c->s))) { + continue; + } + /* Skip anything we would count as moribund */ if (connection_is_moribund(c)) { continue; diff --git a/src/or/main.c b/src/or/main.c index 6ab94f5e36..a1b5954aaf 100644 --- a/src/or/main.c +++ b/src/or/main.c @@ -662,7 +662,7 @@ connection_count_moribund, (void)) * runs next. */ SMARTLIST_FOREACH_BEGIN(closeable_connection_lst, connection_t *, conn) { - if (connection_is_moribund(conn)) ++moribund; + if (SOCKET_OK(conn->s) && connection_is_moribund(conn)) ++moribund; } SMARTLIST_FOREACH_END(conn); return moribund; From 31028af4404c06088ea1eb47309ce8c3154150f1 Mon Sep 17 00:00:00 2001 From: Andrea Shepard Date: Sat, 20 Aug 2016 04:36:37 +0000 Subject: [PATCH 16/17] Add DisableOOSCheck option, on by default until we have better heuristics --- src/or/config.c | 1 + src/or/connection.c | 5 +++++ src/or/or.h | 3 +++ 3 files changed, 9 insertions(+) diff --git a/src/or/config.c b/src/or/config.c index 4831bad07a..df85bc3534 100644 --- a/src/or/config.c +++ b/src/or/config.c @@ -211,6 +211,7 @@ static config_var_t option_vars_[] = { V(CountPrivateBandwidth, BOOL, "0"), V(DataDirectory, FILENAME, NULL), V(DataDirectoryGroupReadable, BOOL, "0"), + V(DisableOOSCheck, BOOL, "1"), V(DisableNetwork, BOOL, "0"), V(DirAllowPrivateAddresses, BOOL, "0"), V(TestingAuthDirTimeToLearnReachability, INTERVAL, "30 minutes"), diff --git a/src/or/connection.c b/src/or/connection.c index 6b85bfb550..daa11eeb57 100644 --- a/src/or/connection.c +++ b/src/or/connection.c @@ -4673,6 +4673,11 @@ connection_check_oos(int n_socks, int failed) int target_n_socks = 0, moribund_socks, socks_to_kill; smartlist_t *conns; + /* Early exit: is OOS checking disabled? */ + if (get_options()->DisableOOSCheck) { + return; + } + /* Sanity-check args */ tor_assert(n_socks >= 0); diff --git a/src/or/or.h b/src/or/or.h index 262085ea62..7eaeac8ecd 100644 --- a/src/or/or.h +++ b/src/or/or.h @@ -4458,6 +4458,9 @@ typedef struct { * participate in the protocol. If on (default), a flag is added to the * vote indicating participation. */ int AuthDirSharedRandomness; + + /** If 1, we skip all OOS checks. */ + int DisableOOSCheck; } or_options_t; /** Persistent state for an onion router, as saved to disk. */ From 9a09513c0bd54abae3317be4136cf2578e11fbf3 Mon Sep 17 00:00:00 2001 From: Andrea Shepard Date: Sat, 20 Aug 2016 04:43:01 +0000 Subject: [PATCH 17/17] Use connection_mark_for_close() rather than connection_mark_on_flush() on OOS --- src/or/connection.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/or/connection.c b/src/or/connection.c index daa11eeb57..9b583f4243 100644 --- a/src/or/connection.c +++ b/src/or/connection.c @@ -4653,12 +4653,12 @@ kill_conn_list_for_oos, (smartlist_t *conns)) if (c->type == CONN_TYPE_OR) { connection_or_close_for_error(TO_OR_CONN(c), 1); } else { - connection_mark_and_flush(c); + connection_mark_for_close(c); } } SMARTLIST_FOREACH_END(c); log_notice(LD_NET, - "OOS handler marked and flushed %d connections", + "OOS handler marked %d connections", smartlist_len(conns)); }