March 2018 - tor-commits - lists.torproject.org

[tor/release-0.3.3] Remove MaxDownloadTries options from the manpage
by nickm＠torproject.org 03 Mar '18

03 Mar '18

commit 30225fd89a1899cad4f53506773939ca64ead8b0 Author: Nick Mathewson <nickm(a)torproject.org> Date: Wed Jan 31 15:14:48 2018 -0500 Remove MaxDownloadTries options from the manpage --- doc/tor.1.txt | 30 ------------------------------ 1 file changed, 30 deletions(-) diff --git a/doc/tor.1.txt b/doc/tor.1.txt index ef3d1eb9e..024ef1e15 100644 --- a/doc/tor.1.txt +++ b/doc/tor.1.txt @@ -1757,14 +1757,6 @@ The following options are useful only for clients (that is, if which are advanced by connection failures. (Default: 0, 3, 7, 3600, 10800, 25200, 54000, 111600, 262800) -[[ClientBootstrapConsensusMaxDownloadTries]] **ClientBootstrapConsensusMaxDownloadTries** __NUM__:: - Try this many times to download a consensus while bootstrapping using - fallback directory mirrors before giving up. (Default: 7) - -[[ClientBootstrapConsensusAuthorityOnlyMaxDownloadTries]] **ClientBootstrapConsensusAuthorityOnlyMaxDownloadTries** __NUM__:: - Try this many times to download a consensus while bootstrapping using - authorities before giving up. (Default: 4) - [[ClientBootstrapConsensusMaxInProgressTries]] **ClientBootstrapConsensusMaxInProgressTries** __NUM__:: Try this many simultaneous connections to download a consensus before waiting for one to complete, timeout, or error out. (Default: 3) @@ -2776,8 +2768,6 @@ The following options are used for running a testing Tor network. 4 (for 40 seconds), 8, 16, 32, 60 ClientBootstrapConsensusAuthorityOnlyDownloadSchedule 0, 1, 4 (for 40 seconds), 8, 16, 32, 60 - ClientBootstrapConsensusMaxDownloadTries 80 - ClientBootstrapConsensusAuthorityOnlyMaxDownloadTries 80 ClientDNSRejectInternalAddresses 0 ClientRejectInternalAddresses 0 CountPrivateBandwidth 1 @@ -2800,10 +2790,6 @@ The following options are used for running a testing Tor network. TestingBridgeBootstrapDownloadSchedule 0, 0, 5, 10, 15, 20, 30, 60 TestingClientMaxIntervalWithoutRequest 5 seconds TestingDirConnectionMaxStall 30 seconds - TestingConsensusMaxDownloadTries 80 - TestingDescriptorMaxDownloadTries 80 - TestingMicrodescMaxDownloadTries 80 - TestingCertMaxDownloadTries 80 TestingEnableConnBwEvent 1 TestingEnableCellStatsEvent 1 TestingEnableTbEmptyEvent 1 @@ -2884,22 +2870,6 @@ The following options are used for running a testing Tor network. Changing this requires that **TestingTorNetwork** is set. (Default: 5 minutes) -[[TestingConsensusMaxDownloadTries]] **TestingConsensusMaxDownloadTries** __NUM__:: - Try this many times to download a consensus before giving up. Changing - this requires that **TestingTorNetwork** is set. (Default: 8) - -[[TestingDescriptorMaxDownloadTries]] **TestingDescriptorMaxDownloadTries** __NUM__:: - Try this often to download a server descriptor before giving up. - Changing this requires that **TestingTorNetwork** is set. (Default: 8) - -[[TestingMicrodescMaxDownloadTries]] **TestingMicrodescMaxDownloadTries** __NUM__:: - Try this often to download a microdesc descriptor before giving up. - Changing this requires that **TestingTorNetwork** is set. (Default: 8) - -[[TestingCertMaxDownloadTries]] **TestingCertMaxDownloadTries** __NUM__:: - Try this often to download a v3 authority certificate before giving up. - Changing this requires that **TestingTorNetwork** is set. (Default: 8) - [[TestingDirAuthVoteExit]] **TestingDirAuthVoteExit** __node__,__node__,__...__:: A list of identity fingerprints, country codes, and address patterns of nodes to vote Exit for regardless of their

1 0

[tor/master] Remove the old ("deterministic") download schedule.
by nickm＠torproject.org 03 Mar '18

03 Mar '18

commit e0049ef022b8bf9b808a9074820bb2a33f92ac1b Author: Nick Mathewson <nickm(a)torproject.org> Date: Thu Jan 25 15:51:13 2018 -0500 Remove the old ("deterministic") download schedule. We haven't meant to use it since we introduced the random exponential schedule. Closes ticket 23814. --- changes/refactor23814 | 4 + src/or/bridges.c | 1 - src/or/control.c | 39 +--- src/or/directory.c | 35 +--- src/or/directory.h | 11 +- src/or/networkstatus.c | 8 +- src/or/or.h | 14 -- src/or/routerlist.c | 1 - src/or/routerparse.c | 1 - src/test/test_controller.c | 26 +-- src/test/test_dir.c | 440 +-------------------------------------------- 11 files changed, 46 insertions(+), 534 deletions(-) diff --git a/changes/refactor23814 b/changes/refactor23814 new file mode 100644 index 000000000..75d84bfd1 --- /dev/null +++ b/changes/refactor23814 @@ -0,0 +1,4 @@ + o Code simplification and refactoring: + - Remove the old (deterministic) directory retry logic entirely: + We've used exponential backoff exclusively for some time. + Closes ticket 24814 diff --git a/src/or/bridges.c b/src/or/bridges.c index 383c33fa6..3d9af21fa 100644 --- a/src/or/bridges.c +++ b/src/or/bridges.c @@ -458,7 +458,6 @@ bridge_add_from_config(bridge_line_t *bridge_line) if (bridge_line->transport_name) b->transport_name = bridge_line->transport_name; b->fetch_status.schedule = DL_SCHED_BRIDGE; - b->fetch_status.backoff = DL_SCHED_RANDOM_EXPONENTIAL; b->fetch_status.increment_on = DL_SCHED_INCREMENT_ATTEMPT; /* We can't reset the bridge's download status here, because UseBridges * might be 0 now, and it might be changed to 1 much later. */ diff --git a/src/or/control.c b/src/or/control.c index cce5c7953..1cb6360c8 100644 --- a/src/or/control.c +++ b/src/or/control.c @@ -2252,7 +2252,7 @@ digest_list_to_string(const smartlist_t *sl) static char * download_status_to_string(const download_status_t *dl) { - char *rv = NULL, *tmp; + char *rv = NULL; char tbuf[ISO_TIME_LEN+1]; const char *schedule_str, *want_authority_str; const char *increment_on_str, *backoff_str; @@ -2300,49 +2300,28 @@ download_status_to_string(const download_status_t *dl) break; } - switch (dl->backoff) { - case DL_SCHED_DETERMINISTIC: - backoff_str = "DL_SCHED_DETERMINISTIC"; - break; - case DL_SCHED_RANDOM_EXPONENTIAL: - backoff_str = "DL_SCHED_RANDOM_EXPONENTIAL"; - break; - default: - backoff_str = "unknown"; - break; - } + backoff_str = "DL_SCHED_RANDOM_EXPONENTIAL"; /* Now assemble them */ - tor_asprintf(&tmp, + tor_asprintf(&rv, "next-attempt-at %s\n" "n-download-failures %u\n" "n-download-attempts %u\n" "schedule %s\n" "want-authority %s\n" "increment-on %s\n" - "backoff %s\n", + "backoff %s\n" + "last-backoff-position %u\n" + "last-delay-used %d\n", tbuf, dl->n_download_failures, dl->n_download_attempts, schedule_str, want_authority_str, increment_on_str, - backoff_str); - - if (dl->backoff == DL_SCHED_RANDOM_EXPONENTIAL) { - /* Additional fields become relevant in random-exponential mode */ - tor_asprintf(&rv, - "%s" - "last-backoff-position %u\n" - "last-delay-used %d\n", - tmp, - dl->last_backoff_position, - dl->last_delay_used); - tor_free(tmp); - } else { - /* That was it */ - rv = tmp; - } + backoff_str, + dl->last_backoff_position, + dl->last_delay_used); } return rv; diff --git a/src/or/directory.c b/src/or/directory.c index c55f81bc6..a2e514e79 100644 --- a/src/or/directory.c +++ b/src/or/directory.c @@ -5383,10 +5383,7 @@ find_dl_min_and_max_delay(download_status_t *dls, const or_options_t *options, *min = *((int *)(smartlist_get(schedule, 0))); /* Increment on failure schedules always use exponential backoff, but they * have a smaller limit when they're deterministic */ - if (dls->backoff == DL_SCHED_DETERMINISTIC) - *max = *((int *)((smartlist_get(schedule, smartlist_len(schedule) - 1)))); - else - *max = INT_MAX; + *max = INT_MAX; } /** As next_random_exponential_delay() below, but does not compute a random @@ -5453,26 +5450,20 @@ next_random_exponential_delay(int delay, return MIN(rand_delay, max_delay); } -/** Find the current delay for dls based on schedule or min_delay/ - * max_delay if we're using exponential backoff. If dls->backoff is - * DL_SCHED_RANDOM_EXPONENTIAL, we must have 0 <= min_delay <= max_delay <= - * INT_MAX, but schedule may be set to NULL; otherwise schedule is required. +/** Find the current delay for dls based on min_delay/ + * max_delay. Requires that 0 <= min_delay <= max_delay <= INT_MAX. + * * This function sets dls->next_attempt_at based on now, and returns the delay. * Helper for download_status_increment_failure and * download_status_increment_attempt. */ STATIC int download_status_schedule_get_delay(download_status_t *dls, - const smartlist_t *schedule, int min_delay, int max_delay, time_t now) { tor_assert(dls); - /* We don't need a schedule if we're using random exponential backoff */ - tor_assert(dls->backoff == DL_SCHED_RANDOM_EXPONENTIAL || - schedule != NULL); /* If we're using random exponential backoff, we do need min/max delay */ - tor_assert(dls->backoff != DL_SCHED_RANDOM_EXPONENTIAL || - (min_delay >= 0 && max_delay >= min_delay)); + tor_assert(min_delay >= 0 && max_delay >= min_delay); int delay = INT_MAX; uint8_t dls_schedule_position = (dls->increment_on @@ -5480,14 +5471,8 @@ download_status_schedule_get_delay(download_status_t *dls, ? dls->n_download_attempts : dls->n_download_failures); - if (dls->backoff == DL_SCHED_DETERMINISTIC) { - if (dls_schedule_position < smartlist_len(schedule)) - delay = *(int *)smartlist_get(schedule, dls_schedule_position); - else if (dls_schedule_position == IMPOSSIBLE_TO_DOWNLOAD) - delay = INT_MAX; - else - delay = *(int *)smartlist_get(schedule, smartlist_len(schedule) - 1); - } else if (dls->backoff == DL_SCHED_RANDOM_EXPONENTIAL) { + { // XXXX 23814 unindent. + /* Check if we missed a reset somehow */ IF_BUG_ONCE(dls->last_backoff_position > dls_schedule_position) { dls->last_backoff_position = 0; @@ -5603,9 +5588,8 @@ download_status_increment_failure(download_status_t *dls, int status_code, /* only return a failure retry time if this schedule increments on failures */ - const smartlist_t *schedule = find_dl_schedule(dls, get_options()); find_dl_min_and_max_delay(dls, get_options(), &min_delay, &max_delay); - increment = download_status_schedule_get_delay(dls, schedule, + increment = download_status_schedule_get_delay(dls, min_delay, max_delay, now); } @@ -5657,9 +5641,8 @@ download_status_increment_attempt(download_status_t *dls, const char *item, if (dls->n_download_attempts < IMPOSSIBLE_TO_DOWNLOAD-1) ++dls->n_download_attempts; - const smartlist_t *schedule = find_dl_schedule(dls, get_options()); find_dl_min_and_max_delay(dls, get_options(), &min_delay, &max_delay); - delay = download_status_schedule_get_delay(dls, schedule, + delay = download_status_schedule_get_delay(dls, min_delay, max_delay, now); download_status_log_helper(item, dls->increment_on, "attempted", diff --git a/src/or/directory.h b/src/or/directory.h index edf75ffe1..6008d3a88 100644 --- a/src/or/directory.h +++ b/src/or/directory.h @@ -142,19 +142,13 @@ static inline int download_status_is_ready(download_status_t *dls, time_t now, int max_failures) { + (void) max_failures; // 23814 REMOVE + /* dls wasn't reset before it was used */ if (dls->next_attempt_at == 0) { download_status_reset(dls); } - if (dls->backoff == DL_SCHED_DETERMINISTIC) { - /* Deterministic schedules can hit an endpoint; exponential backoff - * schedules just wait longer and longer. */ - int under_failure_limit = (dls->n_download_failures <= max_failures - && dls->n_download_attempts <= max_failures); - if (!under_failure_limit) - return 0; - } return download_status_get_next_attempt_at(dls) <= now; } @@ -260,7 +254,6 @@ MOCK_DECL(STATIC int, directory_handle_command_post,(dir_connection_t *conn, const char *body, size_t body_len)); STATIC int download_status_schedule_get_delay(download_status_t *dls, - const smartlist_t *schedule, int min_delay, int max_delay, time_t now); diff --git a/src/or/networkstatus.c b/src/or/networkstatus.c index aa0a8d15c..e051e41f5 100644 --- a/src/or/networkstatus.c +++ b/src/or/networkstatus.c @@ -106,9 +106,9 @@ static time_t time_to_download_next_consensus[N_CONSENSUS_FLAVORS]; static download_status_t consensus_dl_status[N_CONSENSUS_FLAVORS] = { { 0, 0, 0, DL_SCHED_CONSENSUS, DL_WANT_ANY_DIRSERVER, - DL_SCHED_INCREMENT_FAILURE, DL_SCHED_RANDOM_EXPONENTIAL, 0, 0 }, + DL_SCHED_INCREMENT_FAILURE, 0, 0 }, { 0, 0, 0, DL_SCHED_CONSENSUS, DL_WANT_ANY_DIRSERVER, - DL_SCHED_INCREMENT_FAILURE, DL_SCHED_RANDOM_EXPONENTIAL, 0, 0 }, + DL_SCHED_INCREMENT_FAILURE, 0, 0 }, }; #define N_CONSENSUS_BOOTSTRAP_SCHEDULES 2 @@ -125,10 +125,10 @@ static download_status_t consensus_bootstrap_dl_status[N_CONSENSUS_BOOTSTRAP_SCHEDULES] = { { 0, 0, 0, DL_SCHED_CONSENSUS, DL_WANT_AUTHORITY, - DL_SCHED_INCREMENT_ATTEMPT, DL_SCHED_RANDOM_EXPONENTIAL, 0, 0 }, + DL_SCHED_INCREMENT_ATTEMPT, 0, 0 }, /* During bootstrap, DL_WANT_ANY_DIRSERVER means "use fallbacks". */ { 0, 0, 0, DL_SCHED_CONSENSUS, DL_WANT_ANY_DIRSERVER, - DL_SCHED_INCREMENT_ATTEMPT, DL_SCHED_RANDOM_EXPONENTIAL, 0, 0 }, + DL_SCHED_INCREMENT_ATTEMPT, 0, 0 }, }; /** True iff we have logged a warning about this OR's version being older than diff --git a/src/or/or.h b/src/or/or.h index c81e29c95..f93050dfe 100644 --- a/src/or/or.h +++ b/src/or/or.h @@ -2069,15 +2069,6 @@ typedef enum { #define download_schedule_increment_bitfield_t \ ENUM_BF(download_schedule_increment_t) -/** Enumeration: do we want to use the random exponential backoff - * mechanism? */ -typedef enum { - DL_SCHED_DETERMINISTIC = 0, - DL_SCHED_RANDOM_EXPONENTIAL = 1, -} download_schedule_backoff_t; -#define download_schedule_backoff_bitfield_t \ - ENUM_BF(download_schedule_backoff_t) - /** Information about our plans for retrying downloads for a downloadable * directory object. * Each type of downloadable directory object has a corresponding retry @@ -2124,11 +2115,6 @@ typedef struct download_status_t { download_schedule_increment_bitfield_t increment_on : 1; /**< does this * schedule increment on each attempt, * or after each failure? */ - download_schedule_backoff_bitfield_t backoff : 1; /**< do we use the - * deterministic schedule, or random - * exponential backoffs? - * Increment on failure schedules - * always use exponential backoff. */ uint8_t last_backoff_position; /**< number of attempts/failures, depending * on increment_on, when we last recalculated * the delay. Only updated if backoff diff --git a/src/or/routerlist.c b/src/or/routerlist.c index 2815c6096..fec224fad 100644 --- a/src/or/routerlist.c +++ b/src/or/routerlist.c @@ -246,7 +246,6 @@ download_status_cert_init(download_status_t *dlstatus) dlstatus->schedule = DL_SCHED_CONSENSUS; dlstatus->want_authority = DL_WANT_ANY_DIRSERVER; dlstatus->increment_on = DL_SCHED_INCREMENT_FAILURE; - dlstatus->backoff = DL_SCHED_RANDOM_EXPONENTIAL; dlstatus->last_backoff_position = 0; dlstatus->last_delay_used = 0; diff --git a/src/or/routerparse.c b/src/or/routerparse.c index f1895ce31..e25860a67 100644 --- a/src/or/routerparse.c +++ b/src/or/routerparse.c @@ -3781,7 +3781,6 @@ networkstatus_parse_vote_from_string(const char *s, const char **eos_out, ns->consensus_method, flav))) { /* Use exponential-backoff scheduling when downloading microdescs */ - rs->dl_status.backoff = DL_SCHED_RANDOM_EXPONENTIAL; smartlist_add(ns->routerstatus_list, rs); } } diff --git a/src/test/test_controller.c b/src/test/test_controller.c index af19f63f6..1c285bb3a 100644 --- a/src/test/test_controller.c +++ b/src/test/test_controller.c @@ -396,7 +396,7 @@ test_add_onion_helper_clientauth(void *arg) static const download_status_t dl_status_default = { 0, 0, 0, DL_SCHED_CONSENSUS, DL_WANT_ANY_DIRSERVER, - DL_SCHED_INCREMENT_FAILURE, DL_SCHED_RANDOM_EXPONENTIAL, 0, 0 }; + DL_SCHED_INCREMENT_FAILURE, 0, 0 }; static download_status_t ns_dl_status[N_CONSENSUS_FLAVORS]; static download_status_t ns_dl_status_bootstrap[N_CONSENSUS_FLAVORS]; static download_status_t ns_dl_status_running[N_CONSENSUS_FLAVORS]; @@ -407,7 +407,7 @@ static download_status_t ns_dl_status_running[N_CONSENSUS_FLAVORS]; */ static const download_status_t dls_sample_1 = { 1467163900, 0, 0, DL_SCHED_GENERIC, DL_WANT_ANY_DIRSERVER, - DL_SCHED_INCREMENT_FAILURE, DL_SCHED_DETERMINISTIC, 0, 0 }; + DL_SCHED_INCREMENT_FAILURE, 0, 0 }; static const char * dls_sample_1_str = "next-attempt-at 2016-06-29 01:31:40\n" "n-download-failures 0\n" @@ -415,10 +415,12 @@ static const char * dls_sample_1_str = "schedule DL_SCHED_GENERIC\n" "want-authority DL_WANT_ANY_DIRSERVER\n" "increment-on DL_SCHED_INCREMENT_FAILURE\n" - "backoff DL_SCHED_DETERMINISTIC\n"; + "backoff DL_SCHED_RANDOM_EXPONENTIAL\n" + "last-backoff-position 0\n" + "last-delay-used 0\n"; static const download_status_t dls_sample_2 = { 1467164400, 1, 2, DL_SCHED_CONSENSUS, DL_WANT_AUTHORITY, - DL_SCHED_INCREMENT_FAILURE, DL_SCHED_DETERMINISTIC, 0, 0 }; + DL_SCHED_INCREMENT_FAILURE, 0, 0 }; static const char * dls_sample_2_str = "next-attempt-at 2016-06-29 01:40:00\n" "n-download-failures 1\n" @@ -426,10 +428,12 @@ static const char * dls_sample_2_str = "schedule DL_SCHED_CONSENSUS\n" "want-authority DL_WANT_AUTHORITY\n" "increment-on DL_SCHED_INCREMENT_FAILURE\n" - "backoff DL_SCHED_DETERMINISTIC\n"; + "backoff DL_SCHED_RANDOM_EXPONENTIAL\n" + "last-backoff-position 0\n" + "last-delay-used 0\n"; static const download_status_t dls_sample_3 = { 1467154400, 12, 25, DL_SCHED_BRIDGE, DL_WANT_ANY_DIRSERVER, - DL_SCHED_INCREMENT_ATTEMPT, DL_SCHED_DETERMINISTIC, 0, 0 }; + DL_SCHED_INCREMENT_ATTEMPT, 0, 0 }; static const char * dls_sample_3_str = "next-attempt-at 2016-06-28 22:53:20\n" "n-download-failures 12\n" @@ -437,10 +441,12 @@ static const char * dls_sample_3_str = "schedule DL_SCHED_BRIDGE\n" "want-authority DL_WANT_ANY_DIRSERVER\n" "increment-on DL_SCHED_INCREMENT_ATTEMPT\n" - "backoff DL_SCHED_DETERMINISTIC\n"; + "backoff DL_SCHED_RANDOM_EXPONENTIAL\n" + "last-backoff-position 0\n" + "last-delay-used 0\n"; static const download_status_t dls_sample_4 = { 1467166600, 3, 0, DL_SCHED_GENERIC, DL_WANT_ANY_DIRSERVER, - DL_SCHED_INCREMENT_FAILURE, DL_SCHED_RANDOM_EXPONENTIAL, 0, 0 }; + DL_SCHED_INCREMENT_FAILURE, 0, 0 }; static const char * dls_sample_4_str = "next-attempt-at 2016-06-29 02:16:40\n" "n-download-failures 3\n" @@ -453,7 +459,7 @@ static const char * dls_sample_4_str = "last-delay-used 0\n"; static const download_status_t dls_sample_5 = { 1467164600, 3, 7, DL_SCHED_CONSENSUS, DL_WANT_ANY_DIRSERVER, - DL_SCHED_INCREMENT_FAILURE, DL_SCHED_RANDOM_EXPONENTIAL, 1, 2112, }; + DL_SCHED_INCREMENT_FAILURE, 1, 2112, }; static const char * dls_sample_5_str = "next-attempt-at 2016-06-29 01:43:20\n" "n-download-failures 3\n" @@ -466,7 +472,7 @@ static const char * dls_sample_5_str = "last-delay-used 2112\n"; static const download_status_t dls_sample_6 = { 1467164200, 4, 9, DL_SCHED_CONSENSUS, DL_WANT_AUTHORITY, - DL_SCHED_INCREMENT_ATTEMPT, DL_SCHED_RANDOM_EXPONENTIAL, 3, 432 }; + DL_SCHED_INCREMENT_ATTEMPT, 3, 432 }; static const char * dls_sample_6_str = "next-attempt-at 2016-06-29 01:36:40\n" "n-download-failures 4\n" diff --git a/src/test/test_dir.c b/src/test/test_dir.c index f2223ee17..dc8d2a3bc 100644 --- a/src/test/test_dir.c +++ b/src/test/test_dir.c @@ -3965,164 +3965,11 @@ test_dir_packages(void *arg) } static void -test_dir_download_status_schedule(void *arg) -{ - (void)arg; - download_status_t dls_failure = { 0, 0, 0, DL_SCHED_GENERIC, - DL_WANT_AUTHORITY, - DL_SCHED_INCREMENT_FAILURE, - DL_SCHED_DETERMINISTIC, 0, 0 }; - download_status_t dls_attempt = { 0, 0, 0, DL_SCHED_CONSENSUS, - DL_WANT_ANY_DIRSERVER, - DL_SCHED_INCREMENT_ATTEMPT, - DL_SCHED_DETERMINISTIC, 0, 0 }; - download_status_t dls_bridge = { 0, 0, 0, DL_SCHED_BRIDGE, - DL_WANT_AUTHORITY, - DL_SCHED_INCREMENT_FAILURE, - DL_SCHED_DETERMINISTIC, 0, 0 }; - int increment = -1; - int expected_increment = -1; - time_t current_time = time(NULL); - int delay1 = -1; - int delay2 = -1; - smartlist_t *schedule = smartlist_new(); - - /* Make a dummy schedule */ - smartlist_add(schedule, (void *)&delay1); - smartlist_add(schedule, (void *)&delay2); - - /* check a range of values */ - delay1 = 1000; - increment = download_status_schedule_get_delay(&dls_failure, - schedule, - 0, INT_MAX, - TIME_MIN); - expected_increment = delay1; - tt_assert(increment == expected_increment); - tt_assert(dls_failure.next_attempt_at == TIME_MIN + expected_increment); - - delay1 = INT_MAX; - increment = download_status_schedule_get_delay(&dls_failure, - schedule, - 0, INT_MAX, - -1); - expected_increment = delay1; - tt_assert(increment == expected_increment); - tt_assert(dls_failure.next_attempt_at == TIME_MAX); - - delay1 = 0; - increment = download_status_schedule_get_delay(&dls_attempt, - schedule, - 0, INT_MAX, - 0); - expected_increment = delay1; - tt_assert(increment == expected_increment); - tt_assert(dls_attempt.next_attempt_at == 0 + expected_increment); - - delay1 = 1000; - increment = download_status_schedule_get_delay(&dls_attempt, - schedule, - 0, INT_MAX, - 1); - expected_increment = delay1; - tt_assert(increment == expected_increment); - tt_assert(dls_attempt.next_attempt_at == 1 + expected_increment); - - delay1 = INT_MAX; - increment = download_status_schedule_get_delay(&dls_bridge, - schedule, - 0, INT_MAX, - current_time); - expected_increment = delay1; - tt_assert(increment == expected_increment); - tt_assert(dls_bridge.next_attempt_at == TIME_MAX); - - delay1 = 1; - increment = download_status_schedule_get_delay(&dls_bridge, - schedule, - 0, INT_MAX, - TIME_MAX); - expected_increment = delay1; - tt_assert(increment == expected_increment); - tt_assert(dls_bridge.next_attempt_at == TIME_MAX); - - /* see what happens when we reach the end */ - dls_attempt.n_download_attempts++; - dls_bridge.n_download_failures++; - - delay2 = 100; - increment = download_status_schedule_get_delay(&dls_attempt, - schedule, - 0, INT_MAX, - current_time); - expected_increment = delay2; - tt_assert(increment == expected_increment); - tt_assert(dls_attempt.next_attempt_at == current_time + delay2); - - delay2 = 1; - increment = download_status_schedule_get_delay(&dls_bridge, - schedule, - 0, INT_MAX, - current_time); - expected_increment = delay2; - tt_assert(increment == expected_increment); - tt_assert(dls_bridge.next_attempt_at == current_time + delay2); - - /* see what happens when we try to go off the end */ - dls_attempt.n_download_attempts++; - dls_bridge.n_download_failures++; - - delay2 = 5; - increment = download_status_schedule_get_delay(&dls_attempt, - schedule, - 0, INT_MAX, - current_time); - expected_increment = delay2; - tt_assert(increment == expected_increment); - tt_assert(dls_attempt.next_attempt_at == current_time + delay2); - - delay2 = 17; - increment = download_status_schedule_get_delay(&dls_bridge, - schedule, - 0, INT_MAX, - current_time); - expected_increment = delay2; - tt_assert(increment == expected_increment); - tt_assert(dls_bridge.next_attempt_at == current_time + delay2); - - /* see what happens when we reach IMPOSSIBLE_TO_DOWNLOAD */ - dls_attempt.n_download_attempts = IMPOSSIBLE_TO_DOWNLOAD; - dls_bridge.n_download_failures = IMPOSSIBLE_TO_DOWNLOAD; - - delay2 = 35; - increment = download_status_schedule_get_delay(&dls_attempt, - schedule, - 0, INT_MAX, - current_time); - expected_increment = INT_MAX; - tt_assert(increment == expected_increment); - tt_assert(dls_attempt.next_attempt_at == TIME_MAX); - - delay2 = 99; - increment = download_status_schedule_get_delay(&dls_bridge, - schedule, - 0, INT_MAX, - current_time); - expected_increment = INT_MAX; - tt_assert(increment == expected_increment); - tt_assert(dls_bridge.next_attempt_at == TIME_MAX); - - done: - /* the pointers in schedule are allocated on the stack */ - smartlist_free(schedule); -} - -static void download_status_random_backoff_helper(int min_delay, int max_delay) { download_status_t dls_random = { 0, 0, 0, DL_SCHED_GENERIC, DL_WANT_AUTHORITY, - DL_SCHED_INCREMENT_FAILURE, DL_SCHED_RANDOM_EXPONENTIAL, 0, 0 }; + DL_SCHED_INCREMENT_FAILURE, 0, 0 }; int increment = -1; int old_increment = -1; time_t current_time = time(NULL); @@ -4131,7 +3978,6 @@ download_status_random_backoff_helper(int min_delay, int max_delay) int n_attempts = 0; do { increment = download_status_schedule_get_delay(&dls_random, - NULL, min_delay, max_delay, current_time); @@ -4220,18 +4066,10 @@ static void test_dir_download_status_increment(void *arg) { (void)arg; - download_status_t dls_failure = { 0, 0, 0, DL_SCHED_GENERIC, - DL_WANT_AUTHORITY, - DL_SCHED_INCREMENT_FAILURE, - DL_SCHED_DETERMINISTIC, 0, 0 }; - download_status_t dls_attempt = { 0, 0, 0, DL_SCHED_BRIDGE, - DL_WANT_ANY_DIRSERVER, - DL_SCHED_INCREMENT_ATTEMPT, - DL_SCHED_DETERMINISTIC, 0, 0 }; download_status_t dls_exp = { 0, 0, 0, DL_SCHED_GENERIC, DL_WANT_ANY_DIRSERVER, DL_SCHED_INCREMENT_ATTEMPT, - DL_SCHED_RANDOM_EXPONENTIAL, 0, 0 }; + 0, 0 }; int no_delay = 0; int delay0 = -1; int delay1 = -1; @@ -4239,7 +4077,6 @@ test_dir_download_status_increment(void *arg) smartlist_t *schedule = smartlist_new(); smartlist_t *schedule_no_initial_delay = smartlist_new(); or_options_t test_options; - time_t next_at = TIME_MAX; time_t current_time = time(NULL); /* Provide some values for the schedules */ @@ -4272,26 +4109,6 @@ test_dir_download_status_increment(void *arg) mock_get_options_calls = 0; /* we really want to test that it's equal to time(NULL) + delay0, but that's * an unrealiable test, because time(NULL) might change. */ - tt_assert(download_status_get_next_attempt_at(&dls_failure) - >= current_time + no_delay); - tt_assert(download_status_get_next_attempt_at(&dls_failure) - != TIME_MAX); - tt_int_op(download_status_get_n_failures(&dls_failure), OP_EQ, 0); - tt_int_op(download_status_get_n_attempts(&dls_failure), OP_EQ, 0); - tt_int_op(mock_get_options_calls, OP_GE, 1); - - /* regression test for 17750: initial delay */ - mock_options->TestingClientDownloadSchedule = schedule; - mock_get_options_calls = 0; - /* we really want to test that it's equal to time(NULL) + delay0, but that's - * an unrealiable test, because time(NULL) might change. */ - tt_assert(download_status_get_next_attempt_at(&dls_failure) - >= current_time + delay0); - tt_assert(download_status_get_next_attempt_at(&dls_failure) - != TIME_MAX); - tt_int_op(download_status_get_n_failures(&dls_failure), OP_EQ, 0); - tt_int_op(download_status_get_n_attempts(&dls_failure), OP_EQ, 0); - tt_int_op(mock_get_options_calls, OP_GE, 1); /* regression test for 17750: exponential, no initial delay */ mock_options->TestingClientDownloadSchedule = schedule_no_initial_delay; @@ -4319,258 +4136,6 @@ test_dir_download_status_increment(void *arg) tt_int_op(download_status_get_n_attempts(&dls_exp), OP_EQ, 0); tt_int_op(mock_get_options_calls, OP_GE, 1); - /* Check that a failure reset works */ - mock_get_options_calls = 0; - download_status_reset(&dls_failure); - /* we really want to test that it's equal to time(NULL) + delay0, but that's - * an unrealiable test, because time(NULL) might change. */ - tt_assert(download_status_get_next_attempt_at(&dls_failure) - >= current_time + delay0); - tt_assert(download_status_get_next_attempt_at(&dls_failure) - != TIME_MAX); - tt_int_op(download_status_get_n_failures(&dls_failure), OP_EQ, 0); - tt_int_op(download_status_get_n_attempts(&dls_failure), OP_EQ, 0); - tt_int_op(mock_get_options_calls, OP_GE, 1); - - /* avoid timing inconsistencies */ - dls_failure.next_attempt_at = current_time + delay0; - - /* check that a reset schedule becomes ready at the right time */ - tt_int_op(download_status_is_ready(&dls_failure, - current_time + delay0 - 1, 1), - OP_EQ, 0); - tt_int_op(download_status_is_ready(&dls_failure, - current_time + delay0, 1), - OP_EQ, 1); - tt_int_op(download_status_is_ready(&dls_failure, - current_time + delay0 + 1, 1), - OP_EQ, 1); - - /* Check that a failure increment works */ - mock_get_options_calls = 0; - next_at = download_status_increment_failure(&dls_failure, 404, "test", 0, - current_time); - tt_assert(next_at == current_time + delay1); - tt_int_op(download_status_get_n_failures(&dls_failure), OP_EQ, 1); - tt_int_op(download_status_get_n_attempts(&dls_failure), OP_EQ, 1); - tt_int_op(mock_get_options_calls, OP_GE, 1); - - /* check that an incremented schedule becomes ready at the right time */ - tt_int_op(download_status_is_ready(&dls_failure, - current_time + delay1 - 1, 1), - OP_EQ, 0); - tt_int_op(download_status_is_ready(&dls_failure, - current_time + delay1, 1), - OP_EQ, 1); - tt_int_op(download_status_is_ready(&dls_failure, - current_time + delay1 + 1, 1), - OP_EQ, 1); - - /* check that a schedule isn't ready if it's had too many failures */ - tt_int_op(download_status_is_ready(&dls_failure, - current_time + delay1 + 10, 0), - OP_EQ, 0); - - /* Check that failure increments do happen on 503 for clients, and - * attempt increments do too. */ - mock_get_options_calls = 0; - next_at = download_status_increment_failure(&dls_failure, 503, "test", 0, - current_time); - tt_i64_op(next_at, OP_EQ, current_time + delay2); - tt_int_op(download_status_get_n_failures(&dls_failure), OP_EQ, 2); - tt_int_op(download_status_get_n_attempts(&dls_failure), OP_EQ, 2); - tt_int_op(mock_get_options_calls, OP_GE, 1); - - /* Check that failure increments do happen on 503 for servers */ - mock_get_options_calls = 0; - next_at = download_status_increment_failure(&dls_failure, 503, "test", 1, - current_time); - tt_assert(next_at == current_time + delay2); - tt_int_op(download_status_get_n_failures(&dls_failure), OP_EQ, 3); - tt_int_op(download_status_get_n_attempts(&dls_failure), OP_EQ, 3); - tt_int_op(mock_get_options_calls, OP_GE, 1); - - /* Check what happens when we run off the end of the schedule */ - mock_get_options_calls = 0; - next_at = download_status_increment_failure(&dls_failure, 404, "test", 0, - current_time); - tt_assert(next_at == current_time + delay2); - tt_int_op(download_status_get_n_failures(&dls_failure), OP_EQ, 4); - tt_int_op(download_status_get_n_attempts(&dls_failure), OP_EQ, 4); - tt_int_op(mock_get_options_calls, OP_GE, 1); - - /* Check what happens when we hit the failure limit */ - mock_get_options_calls = 0; - download_status_mark_impossible(&dls_failure); - next_at = download_status_increment_failure(&dls_failure, 404, "test", 0, - current_time); - tt_assert(next_at == TIME_MAX); - tt_int_op(download_status_get_n_failures(&dls_failure), OP_EQ, - IMPOSSIBLE_TO_DOWNLOAD); - tt_int_op(download_status_get_n_attempts(&dls_failure), OP_EQ, - IMPOSSIBLE_TO_DOWNLOAD); - tt_int_op(mock_get_options_calls, OP_GE, 1); - - /* Check that a failure reset doesn't reset at the limit */ - mock_get_options_calls = 0; - download_status_reset(&dls_failure); - tt_assert(download_status_get_next_attempt_at(&dls_failure) - == TIME_MAX); - tt_int_op(download_status_get_n_failures(&dls_failure), OP_EQ, - IMPOSSIBLE_TO_DOWNLOAD); - tt_int_op(download_status_get_n_attempts(&dls_failure), OP_EQ, - IMPOSSIBLE_TO_DOWNLOAD); - tt_int_op(mock_get_options_calls, OP_EQ, 0); - - /* Check that a failure reset resets just before the limit */ - mock_get_options_calls = 0; - dls_failure.n_download_failures = IMPOSSIBLE_TO_DOWNLOAD - 1; - dls_failure.n_download_attempts = IMPOSSIBLE_TO_DOWNLOAD - 1; - download_status_reset(&dls_failure); - /* we really want to test that it's equal to time(NULL) + delay0, but that's - * an unrealiable test, because time(NULL) might change. */ - tt_assert(download_status_get_next_attempt_at(&dls_failure) - >= current_time + delay0); - tt_assert(download_status_get_next_attempt_at(&dls_failure) - != TIME_MAX); - tt_int_op(download_status_get_n_failures(&dls_failure), OP_EQ, 0); - tt_int_op(download_status_get_n_attempts(&dls_failure), OP_EQ, 0); - tt_int_op(mock_get_options_calls, OP_GE, 1); - - /* Check that failure increments do happen on attempt-based schedules, - * but that the retry is set at the end of time */ - mock_options->UseBridges = 1; - mock_get_options_calls = 0; - next_at = download_status_increment_failure(&dls_attempt, 404, "test", 0, - current_time); - tt_assert(next_at == TIME_MAX); - tt_int_op(download_status_get_n_failures(&dls_attempt), OP_EQ, 1); - tt_int_op(download_status_get_n_attempts(&dls_attempt), OP_EQ, 0); - tt_int_op(mock_get_options_calls, OP_GE, 1); - - /* Check that an attempt reset works */ - mock_get_options_calls = 0; - download_status_reset(&dls_attempt); - /* we really want to test that it's equal to time(NULL) + delay0, but that's - * an unrealiable test, because time(NULL) might change. */ - tt_assert(download_status_get_next_attempt_at(&dls_attempt) - >= current_time + delay0); - tt_assert(download_status_get_next_attempt_at(&dls_attempt) - != TIME_MAX); - tt_int_op(download_status_get_n_failures(&dls_attempt), OP_EQ, 0); - tt_int_op(download_status_get_n_attempts(&dls_attempt), OP_EQ, 0); - tt_int_op(mock_get_options_calls, OP_GE, 1); - - /* avoid timing inconsistencies */ - dls_attempt.next_attempt_at = current_time + delay0; - - /* check that a reset schedule becomes ready at the right time */ - tt_int_op(download_status_is_ready(&dls_attempt, - current_time + delay0 - 1, 1), - OP_EQ, 0); - tt_int_op(download_status_is_ready(&dls_attempt, - current_time + delay0, 1), - OP_EQ, 1); - tt_int_op(download_status_is_ready(&dls_attempt, - current_time + delay0 + 1, 1), - OP_EQ, 1); - - /* Check that an attempt increment works */ - mock_get_options_calls = 0; - next_at = download_status_increment_attempt(&dls_attempt, "test", - current_time); - tt_assert(next_at == current_time + delay1); - tt_int_op(download_status_get_n_failures(&dls_attempt), OP_EQ, 0); - tt_int_op(download_status_get_n_attempts(&dls_attempt), OP_EQ, 1); - tt_int_op(mock_get_options_calls, OP_GE, 1); - - /* check that an incremented schedule becomes ready at the right time */ - tt_int_op(download_status_is_ready(&dls_attempt, - current_time + delay1 - 1, 1), - OP_EQ, 0); - tt_int_op(download_status_is_ready(&dls_attempt, - current_time + delay1, 1), - OP_EQ, 1); - tt_int_op(download_status_is_ready(&dls_attempt, - current_time + delay1 + 1, 1), - OP_EQ, 1); - - /* check that a schedule isn't ready if it's had too many attempts */ - tt_int_op(download_status_is_ready(&dls_attempt, - current_time + delay1 + 10, 0), - OP_EQ, 0); - - /* Check what happens when we reach then run off the end of the schedule */ - mock_get_options_calls = 0; - next_at = download_status_increment_attempt(&dls_attempt, "test", - current_time); - tt_assert(next_at == current_time + delay2); - tt_int_op(download_status_get_n_failures(&dls_attempt), OP_EQ, 0); - tt_int_op(download_status_get_n_attempts(&dls_attempt), OP_EQ, 2); - tt_int_op(mock_get_options_calls, OP_GE, 1); - - mock_get_options_calls = 0; - next_at = download_status_increment_attempt(&dls_attempt, "test", - current_time); - tt_assert(next_at == current_time + delay2); - tt_int_op(download_status_get_n_failures(&dls_attempt), OP_EQ, 0); - tt_int_op(download_status_get_n_attempts(&dls_attempt), OP_EQ, 3); - tt_int_op(mock_get_options_calls, OP_GE, 1); - - /* Check what happens when we hit the attempt limit */ - mock_get_options_calls = 0; - download_status_mark_impossible(&dls_attempt); - next_at = download_status_increment_attempt(&dls_attempt, "test", - current_time); - tt_assert(next_at == TIME_MAX); - tt_int_op(download_status_get_n_failures(&dls_attempt), OP_EQ, - IMPOSSIBLE_TO_DOWNLOAD); - tt_int_op(download_status_get_n_attempts(&dls_attempt), OP_EQ, - IMPOSSIBLE_TO_DOWNLOAD); - tt_int_op(mock_get_options_calls, OP_GE, 1); - - /* Check that an attempt reset doesn't reset at the limit */ - mock_get_options_calls = 0; - download_status_reset(&dls_attempt); - tt_assert(download_status_get_next_attempt_at(&dls_attempt) - == TIME_MAX); - tt_int_op(download_status_get_n_failures(&dls_attempt), OP_EQ, - IMPOSSIBLE_TO_DOWNLOAD); - tt_int_op(download_status_get_n_attempts(&dls_attempt), OP_EQ, - IMPOSSIBLE_TO_DOWNLOAD); - tt_int_op(mock_get_options_calls, OP_EQ, 0); - - /* Check that an attempt reset resets just before the limit */ - mock_get_options_calls = 0; - dls_attempt.n_download_failures = IMPOSSIBLE_TO_DOWNLOAD - 1; - dls_attempt.n_download_attempts = IMPOSSIBLE_TO_DOWNLOAD - 1; - download_status_reset(&dls_attempt); - /* we really want to test that it's equal to time(NULL) + delay0, but that's - * an unrealiable test, because time(NULL) might change. */ - tt_assert(download_status_get_next_attempt_at(&dls_attempt) - >= current_time + delay0); - tt_assert(download_status_get_next_attempt_at(&dls_attempt) - != TIME_MAX); - tt_int_op(download_status_get_n_failures(&dls_attempt), OP_EQ, 0); - tt_int_op(download_status_get_n_attempts(&dls_attempt), OP_EQ, 0); - tt_int_op(mock_get_options_calls, OP_GE, 1); - mock_options->UseBridges = 0; - - /* Check that attempt increments don't happen on failure-based schedules, - * and that the attempt is set at the end of time */ - mock_get_options_calls = 0; - setup_full_capture_of_logs(LOG_WARN); - next_at = download_status_increment_attempt(&dls_failure, "test", - current_time); - expect_single_log_msg_containing( - "Tried to launch an attempt-based connection on a failure-based " - "schedule."); - teardown_capture_of_logs(); - tt_assert(next_at == TIME_MAX); - tt_int_op(download_status_get_n_failures(&dls_failure), OP_EQ, 0); - tt_int_op(download_status_get_n_attempts(&dls_failure), OP_EQ, 0); - tt_int_op(mock_get_options_calls, OP_EQ, 0); - done: /* the pointers in schedule are allocated on the stack */ smartlist_free(schedule); @@ -6318,7 +5883,6 @@ struct testcase_t dir_tests[] = { DIR(post_parsing, 0), DIR(fetch_type, 0), DIR(packages, 0), - DIR(download_status_schedule, 0), DIR(download_status_random_backoff, 0), DIR(download_status_random_backoff_ranges, 0), DIR(download_status_increment, TT_FORK),

1 0

[tor/maint-0.3.3] Remove the old ("deterministic") download schedule.
by nickm＠torproject.org 03 Mar '18

03 Mar '18

commit e0049ef022b8bf9b808a9074820bb2a33f92ac1b Author: Nick Mathewson <nickm(a)torproject.org> Date: Thu Jan 25 15:51:13 2018 -0500 Remove the old ("deterministic") download schedule. We haven't meant to use it since we introduced the random exponential schedule. Closes ticket 23814. --- changes/refactor23814 | 4 + src/or/bridges.c | 1 - src/or/control.c | 39 +--- src/or/directory.c | 35 +--- src/or/directory.h | 11 +- src/or/networkstatus.c | 8 +- src/or/or.h | 14 -- src/or/routerlist.c | 1 - src/or/routerparse.c | 1 - src/test/test_controller.c | 26 +-- src/test/test_dir.c | 440 +-------------------------------------------- 11 files changed, 46 insertions(+), 534 deletions(-) diff --git a/changes/refactor23814 b/changes/refactor23814 new file mode 100644 index 000000000..75d84bfd1 --- /dev/null +++ b/changes/refactor23814 @@ -0,0 +1,4 @@ + o Code simplification and refactoring: + - Remove the old (deterministic) directory retry logic entirely: + We've used exponential backoff exclusively for some time. + Closes ticket 24814 diff --git a/src/or/bridges.c b/src/or/bridges.c index 383c33fa6..3d9af21fa 100644 --- a/src/or/bridges.c +++ b/src/or/bridges.c @@ -458,7 +458,6 @@ bridge_add_from_config(bridge_line_t *bridge_line) if (bridge_line->transport_name) b->transport_name = bridge_line->transport_name; b->fetch_status.schedule = DL_SCHED_BRIDGE; - b->fetch_status.backoff = DL_SCHED_RANDOM_EXPONENTIAL; b->fetch_status.increment_on = DL_SCHED_INCREMENT_ATTEMPT; /* We can't reset the bridge's download status here, because UseBridges * might be 0 now, and it might be changed to 1 much later. */ diff --git a/src/or/control.c b/src/or/control.c index cce5c7953..1cb6360c8 100644 --- a/src/or/control.c +++ b/src/or/control.c @@ -2252,7 +2252,7 @@ digest_list_to_string(const smartlist_t *sl) static char * download_status_to_string(const download_status_t *dl) { - char *rv = NULL, *tmp; + char *rv = NULL; char tbuf[ISO_TIME_LEN+1]; const char *schedule_str, *want_authority_str; const char *increment_on_str, *backoff_str; @@ -2300,49 +2300,28 @@ download_status_to_string(const download_status_t *dl) break; } - switch (dl->backoff) { - case DL_SCHED_DETERMINISTIC: - backoff_str = "DL_SCHED_DETERMINISTIC"; - break; - case DL_SCHED_RANDOM_EXPONENTIAL: - backoff_str = "DL_SCHED_RANDOM_EXPONENTIAL"; - break; - default: - backoff_str = "unknown"; - break; - } + backoff_str = "DL_SCHED_RANDOM_EXPONENTIAL"; /* Now assemble them */ - tor_asprintf(&tmp, + tor_asprintf(&rv, "next-attempt-at %s\n" "n-download-failures %u\n" "n-download-attempts %u\n" "schedule %s\n" "want-authority %s\n" "increment-on %s\n" - "backoff %s\n", + "backoff %s\n" + "last-backoff-position %u\n" + "last-delay-used %d\n", tbuf, dl->n_download_failures, dl->n_download_attempts, schedule_str, want_authority_str, increment_on_str, - backoff_str); - - if (dl->backoff == DL_SCHED_RANDOM_EXPONENTIAL) { - /* Additional fields become relevant in random-exponential mode */ - tor_asprintf(&rv, - "%s" - "last-backoff-position %u\n" - "last-delay-used %d\n", - tmp, - dl->last_backoff_position, - dl->last_delay_used); - tor_free(tmp); - } else { - /* That was it */ - rv = tmp; - } + backoff_str, + dl->last_backoff_position, + dl->last_delay_used); } return rv; diff --git a/src/or/directory.c b/src/or/directory.c index c55f81bc6..a2e514e79 100644 --- a/src/or/directory.c +++ b/src/or/directory.c @@ -5383,10 +5383,7 @@ find_dl_min_and_max_delay(download_status_t *dls, const or_options_t *options, *min = *((int *)(smartlist_get(schedule, 0))); /* Increment on failure schedules always use exponential backoff, but they * have a smaller limit when they're deterministic */ - if (dls->backoff == DL_SCHED_DETERMINISTIC) - *max = *((int *)((smartlist_get(schedule, smartlist_len(schedule) - 1)))); - else - *max = INT_MAX; + *max = INT_MAX; } /** As next_random_exponential_delay() below, but does not compute a random @@ -5453,26 +5450,20 @@ next_random_exponential_delay(int delay, return MIN(rand_delay, max_delay); } -/** Find the current delay for dls based on schedule or min_delay/ - * max_delay if we're using exponential backoff. If dls->backoff is - * DL_SCHED_RANDOM_EXPONENTIAL, we must have 0 <= min_delay <= max_delay <= - * INT_MAX, but schedule may be set to NULL; otherwise schedule is required. +/** Find the current delay for dls based on min_delay/ + * max_delay. Requires that 0 <= min_delay <= max_delay <= INT_MAX. + * * This function sets dls->next_attempt_at based on now, and returns the delay. * Helper for download_status_increment_failure and * download_status_increment_attempt. */ STATIC int download_status_schedule_get_delay(download_status_t *dls, - const smartlist_t *schedule, int min_delay, int max_delay, time_t now) { tor_assert(dls); - /* We don't need a schedule if we're using random exponential backoff */ - tor_assert(dls->backoff == DL_SCHED_RANDOM_EXPONENTIAL || - schedule != NULL); /* If we're using random exponential backoff, we do need min/max delay */ - tor_assert(dls->backoff != DL_SCHED_RANDOM_EXPONENTIAL || - (min_delay >= 0 && max_delay >= min_delay)); + tor_assert(min_delay >= 0 && max_delay >= min_delay); int delay = INT_MAX; uint8_t dls_schedule_position = (dls->increment_on @@ -5480,14 +5471,8 @@ download_status_schedule_get_delay(download_status_t *dls, ? dls->n_download_attempts : dls->n_download_failures); - if (dls->backoff == DL_SCHED_DETERMINISTIC) { - if (dls_schedule_position < smartlist_len(schedule)) - delay = *(int *)smartlist_get(schedule, dls_schedule_position); - else if (dls_schedule_position == IMPOSSIBLE_TO_DOWNLOAD) - delay = INT_MAX; - else - delay = *(int *)smartlist_get(schedule, smartlist_len(schedule) - 1); - } else if (dls->backoff == DL_SCHED_RANDOM_EXPONENTIAL) { + { // XXXX 23814 unindent. + /* Check if we missed a reset somehow */ IF_BUG_ONCE(dls->last_backoff_position > dls_schedule_position) { dls->last_backoff_position = 0; @@ -5603,9 +5588,8 @@ download_status_increment_failure(download_status_t *dls, int status_code, /* only return a failure retry time if this schedule increments on failures */ - const smartlist_t *schedule = find_dl_schedule(dls, get_options()); find_dl_min_and_max_delay(dls, get_options(), &min_delay, &max_delay); - increment = download_status_schedule_get_delay(dls, schedule, + increment = download_status_schedule_get_delay(dls, min_delay, max_delay, now); } @@ -5657,9 +5641,8 @@ download_status_increment_attempt(download_status_t *dls, const char *item, if (dls->n_download_attempts < IMPOSSIBLE_TO_DOWNLOAD-1) ++dls->n_download_attempts; - const smartlist_t *schedule = find_dl_schedule(dls, get_options()); find_dl_min_and_max_delay(dls, get_options(), &min_delay, &max_delay); - delay = download_status_schedule_get_delay(dls, schedule, + delay = download_status_schedule_get_delay(dls, min_delay, max_delay, now); download_status_log_helper(item, dls->increment_on, "attempted", diff --git a/src/or/directory.h b/src/or/directory.h index edf75ffe1..6008d3a88 100644 --- a/src/or/directory.h +++ b/src/or/directory.h @@ -142,19 +142,13 @@ static inline int download_status_is_ready(download_status_t *dls, time_t now, int max_failures) { + (void) max_failures; // 23814 REMOVE + /* dls wasn't reset before it was used */ if (dls->next_attempt_at == 0) { download_status_reset(dls); } - if (dls->backoff == DL_SCHED_DETERMINISTIC) { - /* Deterministic schedules can hit an endpoint; exponential backoff - * schedules just wait longer and longer. */ - int under_failure_limit = (dls->n_download_failures <= max_failures - && dls->n_download_attempts <= max_failures); - if (!under_failure_limit) - return 0; - } return download_status_get_next_attempt_at(dls) <= now; } @@ -260,7 +254,6 @@ MOCK_DECL(STATIC int, directory_handle_command_post,(dir_connection_t *conn, const char *body, size_t body_len)); STATIC int download_status_schedule_get_delay(download_status_t *dls, - const smartlist_t *schedule, int min_delay, int max_delay, time_t now); diff --git a/src/or/networkstatus.c b/src/or/networkstatus.c index aa0a8d15c..e051e41f5 100644 --- a/src/or/networkstatus.c +++ b/src/or/networkstatus.c @@ -106,9 +106,9 @@ static time_t time_to_download_next_consensus[N_CONSENSUS_FLAVORS]; static download_status_t consensus_dl_status[N_CONSENSUS_FLAVORS] = { { 0, 0, 0, DL_SCHED_CONSENSUS, DL_WANT_ANY_DIRSERVER, - DL_SCHED_INCREMENT_FAILURE, DL_SCHED_RANDOM_EXPONENTIAL, 0, 0 }, + DL_SCHED_INCREMENT_FAILURE, 0, 0 }, { 0, 0, 0, DL_SCHED_CONSENSUS, DL_WANT_ANY_DIRSERVER, - DL_SCHED_INCREMENT_FAILURE, DL_SCHED_RANDOM_EXPONENTIAL, 0, 0 }, + DL_SCHED_INCREMENT_FAILURE, 0, 0 }, }; #define N_CONSENSUS_BOOTSTRAP_SCHEDULES 2 @@ -125,10 +125,10 @@ static download_status_t consensus_bootstrap_dl_status[N_CONSENSUS_BOOTSTRAP_SCHEDULES] = { { 0, 0, 0, DL_SCHED_CONSENSUS, DL_WANT_AUTHORITY, - DL_SCHED_INCREMENT_ATTEMPT, DL_SCHED_RANDOM_EXPONENTIAL, 0, 0 }, + DL_SCHED_INCREMENT_ATTEMPT, 0, 0 }, /* During bootstrap, DL_WANT_ANY_DIRSERVER means "use fallbacks". */ { 0, 0, 0, DL_SCHED_CONSENSUS, DL_WANT_ANY_DIRSERVER, - DL_SCHED_INCREMENT_ATTEMPT, DL_SCHED_RANDOM_EXPONENTIAL, 0, 0 }, + DL_SCHED_INCREMENT_ATTEMPT, 0, 0 }, }; /** True iff we have logged a warning about this OR's version being older than diff --git a/src/or/or.h b/src/or/or.h index c81e29c95..f93050dfe 100644 --- a/src/or/or.h +++ b/src/or/or.h @@ -2069,15 +2069,6 @@ typedef enum { #define download_schedule_increment_bitfield_t \ ENUM_BF(download_schedule_increment_t) -/** Enumeration: do we want to use the random exponential backoff - * mechanism? */ -typedef enum { - DL_SCHED_DETERMINISTIC = 0, - DL_SCHED_RANDOM_EXPONENTIAL = 1, -} download_schedule_backoff_t; -#define download_schedule_backoff_bitfield_t \ - ENUM_BF(download_schedule_backoff_t) - /** Information about our plans for retrying downloads for a downloadable * directory object. * Each type of downloadable directory object has a corresponding retry @@ -2124,11 +2115,6 @@ typedef struct download_status_t { download_schedule_increment_bitfield_t increment_on : 1; /**< does this * schedule increment on each attempt, * or after each failure? */ - download_schedule_backoff_bitfield_t backoff : 1; /**< do we use the - * deterministic schedule, or random - * exponential backoffs? - * Increment on failure schedules - * always use exponential backoff. */ uint8_t last_backoff_position; /**< number of attempts/failures, depending * on increment_on, when we last recalculated * the delay. Only updated if backoff diff --git a/src/or/routerlist.c b/src/or/routerlist.c index 2815c6096..fec224fad 100644 --- a/src/or/routerlist.c +++ b/src/or/routerlist.c @@ -246,7 +246,6 @@ download_status_cert_init(download_status_t *dlstatus) dlstatus->schedule = DL_SCHED_CONSENSUS; dlstatus->want_authority = DL_WANT_ANY_DIRSERVER; dlstatus->increment_on = DL_SCHED_INCREMENT_FAILURE; - dlstatus->backoff = DL_SCHED_RANDOM_EXPONENTIAL; dlstatus->last_backoff_position = 0; dlstatus->last_delay_used = 0; diff --git a/src/or/routerparse.c b/src/or/routerparse.c index f1895ce31..e25860a67 100644 --- a/src/or/routerparse.c +++ b/src/or/routerparse.c @@ -3781,7 +3781,6 @@ networkstatus_parse_vote_from_string(const char *s, const char **eos_out, ns->consensus_method, flav))) { /* Use exponential-backoff scheduling when downloading microdescs */ - rs->dl_status.backoff = DL_SCHED_RANDOM_EXPONENTIAL; smartlist_add(ns->routerstatus_list, rs); } } diff --git a/src/test/test_controller.c b/src/test/test_controller.c index af19f63f6..1c285bb3a 100644 --- a/src/test/test_controller.c +++ b/src/test/test_controller.c @@ -396,7 +396,7 @@ test_add_onion_helper_clientauth(void *arg) static const download_status_t dl_status_default = { 0, 0, 0, DL_SCHED_CONSENSUS, DL_WANT_ANY_DIRSERVER, - DL_SCHED_INCREMENT_FAILURE, DL_SCHED_RANDOM_EXPONENTIAL, 0, 0 }; + DL_SCHED_INCREMENT_FAILURE, 0, 0 }; static download_status_t ns_dl_status[N_CONSENSUS_FLAVORS]; static download_status_t ns_dl_status_bootstrap[N_CONSENSUS_FLAVORS]; static download_status_t ns_dl_status_running[N_CONSENSUS_FLAVORS]; @@ -407,7 +407,7 @@ static download_status_t ns_dl_status_running[N_CONSENSUS_FLAVORS]; */ static const download_status_t dls_sample_1 = { 1467163900, 0, 0, DL_SCHED_GENERIC, DL_WANT_ANY_DIRSERVER, - DL_SCHED_INCREMENT_FAILURE, DL_SCHED_DETERMINISTIC, 0, 0 }; + DL_SCHED_INCREMENT_FAILURE, 0, 0 }; static const char * dls_sample_1_str = "next-attempt-at 2016-06-29 01:31:40\n" "n-download-failures 0\n" @@ -415,10 +415,12 @@ static const char * dls_sample_1_str = "schedule DL_SCHED_GENERIC\n" "want-authority DL_WANT_ANY_DIRSERVER\n" "increment-on DL_SCHED_INCREMENT_FAILURE\n" - "backoff DL_SCHED_DETERMINISTIC\n"; + "backoff DL_SCHED_RANDOM_EXPONENTIAL\n" + "last-backoff-position 0\n" + "last-delay-used 0\n"; static const download_status_t dls_sample_2 = { 1467164400, 1, 2, DL_SCHED_CONSENSUS, DL_WANT_AUTHORITY, - DL_SCHED_INCREMENT_FAILURE, DL_SCHED_DETERMINISTIC, 0, 0 }; + DL_SCHED_INCREMENT_FAILURE, 0, 0 }; static const char * dls_sample_2_str = "next-attempt-at 2016-06-29 01:40:00\n" "n-download-failures 1\n" @@ -426,10 +428,12 @@ static const char * dls_sample_2_str = "schedule DL_SCHED_CONSENSUS\n" "want-authority DL_WANT_AUTHORITY\n" "increment-on DL_SCHED_INCREMENT_FAILURE\n" - "backoff DL_SCHED_DETERMINISTIC\n"; + "backoff DL_SCHED_RANDOM_EXPONENTIAL\n" + "last-backoff-position 0\n" + "last-delay-used 0\n"; static const download_status_t dls_sample_3 = { 1467154400, 12, 25, DL_SCHED_BRIDGE, DL_WANT_ANY_DIRSERVER, - DL_SCHED_INCREMENT_ATTEMPT, DL_SCHED_DETERMINISTIC, 0, 0 }; + DL_SCHED_INCREMENT_ATTEMPT, 0, 0 }; static const char * dls_sample_3_str = "next-attempt-at 2016-06-28 22:53:20\n" "n-download-failures 12\n" @@ -437,10 +441,12 @@ static const char * dls_sample_3_str = "schedule DL_SCHED_BRIDGE\n" "want-authority DL_WANT_ANY_DIRSERVER\n" "increment-on DL_SCHED_INCREMENT_ATTEMPT\n" - "backoff DL_SCHED_DETERMINISTIC\n"; + "backoff DL_SCHED_RANDOM_EXPONENTIAL\n" + "last-backoff-position 0\n" + "last-delay-used 0\n"; static const download_status_t dls_sample_4 = { 1467166600, 3, 0, DL_SCHED_GENERIC, DL_WANT_ANY_DIRSERVER, - DL_SCHED_INCREMENT_FAILURE, DL_SCHED_RANDOM_EXPONENTIAL, 0, 0 }; + DL_SCHED_INCREMENT_FAILURE, 0, 0 }; static const char * dls_sample_4_str = "next-attempt-at 2016-06-29 02:16:40\n" "n-download-failures 3\n" @@ -453,7 +459,7 @@ static const char * dls_sample_4_str = "last-delay-used 0\n"; static const download_status_t dls_sample_5 = { 1467164600, 3, 7, DL_SCHED_CONSENSUS, DL_WANT_ANY_DIRSERVER, - DL_SCHED_INCREMENT_FAILURE, DL_SCHED_RANDOM_EXPONENTIAL, 1, 2112, }; + DL_SCHED_INCREMENT_FAILURE, 1, 2112, }; static const char * dls_sample_5_str = "next-attempt-at 2016-06-29 01:43:20\n" "n-download-failures 3\n" @@ -466,7 +472,7 @@ static const char * dls_sample_5_str = "last-delay-used 2112\n"; static const download_status_t dls_sample_6 = { 1467164200, 4, 9, DL_SCHED_CONSENSUS, DL_WANT_AUTHORITY, - DL_SCHED_INCREMENT_ATTEMPT, DL_SCHED_RANDOM_EXPONENTIAL, 3, 432 }; + DL_SCHED_INCREMENT_ATTEMPT, 3, 432 }; static const char * dls_sample_6_str = "next-attempt-at 2016-06-29 01:36:40\n" "n-download-failures 4\n" diff --git a/src/test/test_dir.c b/src/test/test_dir.c index f2223ee17..dc8d2a3bc 100644 --- a/src/test/test_dir.c +++ b/src/test/test_dir.c @@ -3965,164 +3965,11 @@ test_dir_packages(void *arg) } static void -test_dir_download_status_schedule(void *arg) -{ - (void)arg; - download_status_t dls_failure = { 0, 0, 0, DL_SCHED_GENERIC, - DL_WANT_AUTHORITY, - DL_SCHED_INCREMENT_FAILURE, - DL_SCHED_DETERMINISTIC, 0, 0 }; - download_status_t dls_attempt = { 0, 0, 0, DL_SCHED_CONSENSUS, - DL_WANT_ANY_DIRSERVER, - DL_SCHED_INCREMENT_ATTEMPT, - DL_SCHED_DETERMINISTIC, 0, 0 }; - download_status_t dls_bridge = { 0, 0, 0, DL_SCHED_BRIDGE, - DL_WANT_AUTHORITY, - DL_SCHED_INCREMENT_FAILURE, - DL_SCHED_DETERMINISTIC, 0, 0 }; - int increment = -1; - int expected_increment = -1; - time_t current_time = time(NULL); - int delay1 = -1; - int delay2 = -1; - smartlist_t *schedule = smartlist_new(); - - /* Make a dummy schedule */ - smartlist_add(schedule, (void *)&delay1); - smartlist_add(schedule, (void *)&delay2); - - /* check a range of values */ - delay1 = 1000; - increment = download_status_schedule_get_delay(&dls_failure, - schedule, - 0, INT_MAX, - TIME_MIN); - expected_increment = delay1; - tt_assert(increment == expected_increment); - tt_assert(dls_failure.next_attempt_at == TIME_MIN + expected_increment); - - delay1 = INT_MAX; - increment = download_status_schedule_get_delay(&dls_failure, - schedule, - 0, INT_MAX, - -1); - expected_increment = delay1; - tt_assert(increment == expected_increment); - tt_assert(dls_failure.next_attempt_at == TIME_MAX); - - delay1 = 0; - increment = download_status_schedule_get_delay(&dls_attempt, - schedule, - 0, INT_MAX, - 0); - expected_increment = delay1; - tt_assert(increment == expected_increment); - tt_assert(dls_attempt.next_attempt_at == 0 + expected_increment); - - delay1 = 1000; - increment = download_status_schedule_get_delay(&dls_attempt, - schedule, - 0, INT_MAX, - 1); - expected_increment = delay1; - tt_assert(increment == expected_increment); - tt_assert(dls_attempt.next_attempt_at == 1 + expected_increment); - - delay1 = INT_MAX; - increment = download_status_schedule_get_delay(&dls_bridge, - schedule, - 0, INT_MAX, - current_time); - expected_increment = delay1; - tt_assert(increment == expected_increment); - tt_assert(dls_bridge.next_attempt_at == TIME_MAX); - - delay1 = 1; - increment = download_status_schedule_get_delay(&dls_bridge, - schedule, - 0, INT_MAX, - TIME_MAX); - expected_increment = delay1; - tt_assert(increment == expected_increment); - tt_assert(dls_bridge.next_attempt_at == TIME_MAX); - - /* see what happens when we reach the end */ - dls_attempt.n_download_attempts++; - dls_bridge.n_download_failures++; - - delay2 = 100; - increment = download_status_schedule_get_delay(&dls_attempt, - schedule, - 0, INT_MAX, - current_time); - expected_increment = delay2; - tt_assert(increment == expected_increment); - tt_assert(dls_attempt.next_attempt_at == current_time + delay2); - - delay2 = 1; - increment = download_status_schedule_get_delay(&dls_bridge, - schedule, - 0, INT_MAX, - current_time); - expected_increment = delay2; - tt_assert(increment == expected_increment); - tt_assert(dls_bridge.next_attempt_at == current_time + delay2); - - /* see what happens when we try to go off the end */ - dls_attempt.n_download_attempts++; - dls_bridge.n_download_failures++; - - delay2 = 5; - increment = download_status_schedule_get_delay(&dls_attempt, - schedule, - 0, INT_MAX, - current_time); - expected_increment = delay2; - tt_assert(increment == expected_increment); - tt_assert(dls_attempt.next_attempt_at == current_time + delay2); - - delay2 = 17; - increment = download_status_schedule_get_delay(&dls_bridge, - schedule, - 0, INT_MAX, - current_time); - expected_increment = delay2; - tt_assert(increment == expected_increment); - tt_assert(dls_bridge.next_attempt_at == current_time + delay2); - - /* see what happens when we reach IMPOSSIBLE_TO_DOWNLOAD */ - dls_attempt.n_download_attempts = IMPOSSIBLE_TO_DOWNLOAD; - dls_bridge.n_download_failures = IMPOSSIBLE_TO_DOWNLOAD; - - delay2 = 35; - increment = download_status_schedule_get_delay(&dls_attempt, - schedule, - 0, INT_MAX, - current_time); - expected_increment = INT_MAX; - tt_assert(increment == expected_increment); - tt_assert(dls_attempt.next_attempt_at == TIME_MAX); - - delay2 = 99; - increment = download_status_schedule_get_delay(&dls_bridge, - schedule, - 0, INT_MAX, - current_time); - expected_increment = INT_MAX; - tt_assert(increment == expected_increment); - tt_assert(dls_bridge.next_attempt_at == TIME_MAX); - - done: - /* the pointers in schedule are allocated on the stack */ - smartlist_free(schedule); -} - -static void download_status_random_backoff_helper(int min_delay, int max_delay) { download_status_t dls_random = { 0, 0, 0, DL_SCHED_GENERIC, DL_WANT_AUTHORITY, - DL_SCHED_INCREMENT_FAILURE, DL_SCHED_RANDOM_EXPONENTIAL, 0, 0 }; + DL_SCHED_INCREMENT_FAILURE, 0, 0 }; int increment = -1; int old_increment = -1; time_t current_time = time(NULL); @@ -4131,7 +3978,6 @@ download_status_random_backoff_helper(int min_delay, int max_delay) int n_attempts = 0; do { increment = download_status_schedule_get_delay(&dls_random, - NULL, min_delay, max_delay, current_time); @@ -4220,18 +4066,10 @@ static void test_dir_download_status_increment(void *arg) { (void)arg; - download_status_t dls_failure = { 0, 0, 0, DL_SCHED_GENERIC, - DL_WANT_AUTHORITY, - DL_SCHED_INCREMENT_FAILURE, - DL_SCHED_DETERMINISTIC, 0, 0 }; - download_status_t dls_attempt = { 0, 0, 0, DL_SCHED_BRIDGE, - DL_WANT_ANY_DIRSERVER, - DL_SCHED_INCREMENT_ATTEMPT, - DL_SCHED_DETERMINISTIC, 0, 0 }; download_status_t dls_exp = { 0, 0, 0, DL_SCHED_GENERIC, DL_WANT_ANY_DIRSERVER, DL_SCHED_INCREMENT_ATTEMPT, - DL_SCHED_RANDOM_EXPONENTIAL, 0, 0 }; + 0, 0 }; int no_delay = 0; int delay0 = -1; int delay1 = -1; @@ -4239,7 +4077,6 @@ test_dir_download_status_increment(void *arg) smartlist_t *schedule = smartlist_new(); smartlist_t *schedule_no_initial_delay = smartlist_new(); or_options_t test_options; - time_t next_at = TIME_MAX; time_t current_time = time(NULL); /* Provide some values for the schedules */ @@ -4272,26 +4109,6 @@ test_dir_download_status_increment(void *arg) mock_get_options_calls = 0; /* we really want to test that it's equal to time(NULL) + delay0, but that's * an unrealiable test, because time(NULL) might change. */ - tt_assert(download_status_get_next_attempt_at(&dls_failure) - >= current_time + no_delay); - tt_assert(download_status_get_next_attempt_at(&dls_failure) - != TIME_MAX); - tt_int_op(download_status_get_n_failures(&dls_failure), OP_EQ, 0); - tt_int_op(download_status_get_n_attempts(&dls_failure), OP_EQ, 0); - tt_int_op(mock_get_options_calls, OP_GE, 1); - - /* regression test for 17750: initial delay */ - mock_options->TestingClientDownloadSchedule = schedule; - mock_get_options_calls = 0; - /* we really want to test that it's equal to time(NULL) + delay0, but that's - * an unrealiable test, because time(NULL) might change. */ - tt_assert(download_status_get_next_attempt_at(&dls_failure) - >= current_time + delay0); - tt_assert(download_status_get_next_attempt_at(&dls_failure) - != TIME_MAX); - tt_int_op(download_status_get_n_failures(&dls_failure), OP_EQ, 0); - tt_int_op(download_status_get_n_attempts(&dls_failure), OP_EQ, 0); - tt_int_op(mock_get_options_calls, OP_GE, 1); /* regression test for 17750: exponential, no initial delay */ mock_options->TestingClientDownloadSchedule = schedule_no_initial_delay; @@ -4319,258 +4136,6 @@ test_dir_download_status_increment(void *arg) tt_int_op(download_status_get_n_attempts(&dls_exp), OP_EQ, 0); tt_int_op(mock_get_options_calls, OP_GE, 1); - /* Check that a failure reset works */ - mock_get_options_calls = 0; - download_status_reset(&dls_failure); - /* we really want to test that it's equal to time(NULL) + delay0, but that's - * an unrealiable test, because time(NULL) might change. */ - tt_assert(download_status_get_next_attempt_at(&dls_failure) - >= current_time + delay0); - tt_assert(download_status_get_next_attempt_at(&dls_failure) - != TIME_MAX); - tt_int_op(download_status_get_n_failures(&dls_failure), OP_EQ, 0); - tt_int_op(download_status_get_n_attempts(&dls_failure), OP_EQ, 0); - tt_int_op(mock_get_options_calls, OP_GE, 1); - - /* avoid timing inconsistencies */ - dls_failure.next_attempt_at = current_time + delay0; - - /* check that a reset schedule becomes ready at the right time */ - tt_int_op(download_status_is_ready(&dls_failure, - current_time + delay0 - 1, 1), - OP_EQ, 0); - tt_int_op(download_status_is_ready(&dls_failure, - current_time + delay0, 1), - OP_EQ, 1); - tt_int_op(download_status_is_ready(&dls_failure, - current_time + delay0 + 1, 1), - OP_EQ, 1); - - /* Check that a failure increment works */ - mock_get_options_calls = 0; - next_at = download_status_increment_failure(&dls_failure, 404, "test", 0, - current_time); - tt_assert(next_at == current_time + delay1); - tt_int_op(download_status_get_n_failures(&dls_failure), OP_EQ, 1); - tt_int_op(download_status_get_n_attempts(&dls_failure), OP_EQ, 1); - tt_int_op(mock_get_options_calls, OP_GE, 1); - - /* check that an incremented schedule becomes ready at the right time */ - tt_int_op(download_status_is_ready(&dls_failure, - current_time + delay1 - 1, 1), - OP_EQ, 0); - tt_int_op(download_status_is_ready(&dls_failure, - current_time + delay1, 1), - OP_EQ, 1); - tt_int_op(download_status_is_ready(&dls_failure, - current_time + delay1 + 1, 1), - OP_EQ, 1); - - /* check that a schedule isn't ready if it's had too many failures */ - tt_int_op(download_status_is_ready(&dls_failure, - current_time + delay1 + 10, 0), - OP_EQ, 0); - - /* Check that failure increments do happen on 503 for clients, and - * attempt increments do too. */ - mock_get_options_calls = 0; - next_at = download_status_increment_failure(&dls_failure, 503, "test", 0, - current_time); - tt_i64_op(next_at, OP_EQ, current_time + delay2); - tt_int_op(download_status_get_n_failures(&dls_failure), OP_EQ, 2); - tt_int_op(download_status_get_n_attempts(&dls_failure), OP_EQ, 2); - tt_int_op(mock_get_options_calls, OP_GE, 1); - - /* Check that failure increments do happen on 503 for servers */ - mock_get_options_calls = 0; - next_at = download_status_increment_failure(&dls_failure, 503, "test", 1, - current_time); - tt_assert(next_at == current_time + delay2); - tt_int_op(download_status_get_n_failures(&dls_failure), OP_EQ, 3); - tt_int_op(download_status_get_n_attempts(&dls_failure), OP_EQ, 3); - tt_int_op(mock_get_options_calls, OP_GE, 1); - - /* Check what happens when we run off the end of the schedule */ - mock_get_options_calls = 0; - next_at = download_status_increment_failure(&dls_failure, 404, "test", 0, - current_time); - tt_assert(next_at == current_time + delay2); - tt_int_op(download_status_get_n_failures(&dls_failure), OP_EQ, 4); - tt_int_op(download_status_get_n_attempts(&dls_failure), OP_EQ, 4); - tt_int_op(mock_get_options_calls, OP_GE, 1); - - /* Check what happens when we hit the failure limit */ - mock_get_options_calls = 0; - download_status_mark_impossible(&dls_failure); - next_at = download_status_increment_failure(&dls_failure, 404, "test", 0, - current_time); - tt_assert(next_at == TIME_MAX); - tt_int_op(download_status_get_n_failures(&dls_failure), OP_EQ, - IMPOSSIBLE_TO_DOWNLOAD); - tt_int_op(download_status_get_n_attempts(&dls_failure), OP_EQ, - IMPOSSIBLE_TO_DOWNLOAD); - tt_int_op(mock_get_options_calls, OP_GE, 1); - - /* Check that a failure reset doesn't reset at the limit */ - mock_get_options_calls = 0; - download_status_reset(&dls_failure); - tt_assert(download_status_get_next_attempt_at(&dls_failure) - == TIME_MAX); - tt_int_op(download_status_get_n_failures(&dls_failure), OP_EQ, - IMPOSSIBLE_TO_DOWNLOAD); - tt_int_op(download_status_get_n_attempts(&dls_failure), OP_EQ, - IMPOSSIBLE_TO_DOWNLOAD); - tt_int_op(mock_get_options_calls, OP_EQ, 0); - - /* Check that a failure reset resets just before the limit */ - mock_get_options_calls = 0; - dls_failure.n_download_failures = IMPOSSIBLE_TO_DOWNLOAD - 1; - dls_failure.n_download_attempts = IMPOSSIBLE_TO_DOWNLOAD - 1; - download_status_reset(&dls_failure); - /* we really want to test that it's equal to time(NULL) + delay0, but that's - * an unrealiable test, because time(NULL) might change. */ - tt_assert(download_status_get_next_attempt_at(&dls_failure) - >= current_time + delay0); - tt_assert(download_status_get_next_attempt_at(&dls_failure) - != TIME_MAX); - tt_int_op(download_status_get_n_failures(&dls_failure), OP_EQ, 0); - tt_int_op(download_status_get_n_attempts(&dls_failure), OP_EQ, 0); - tt_int_op(mock_get_options_calls, OP_GE, 1); - - /* Check that failure increments do happen on attempt-based schedules, - * but that the retry is set at the end of time */ - mock_options->UseBridges = 1; - mock_get_options_calls = 0; - next_at = download_status_increment_failure(&dls_attempt, 404, "test", 0, - current_time); - tt_assert(next_at == TIME_MAX); - tt_int_op(download_status_get_n_failures(&dls_attempt), OP_EQ, 1); - tt_int_op(download_status_get_n_attempts(&dls_attempt), OP_EQ, 0); - tt_int_op(mock_get_options_calls, OP_GE, 1); - - /* Check that an attempt reset works */ - mock_get_options_calls = 0; - download_status_reset(&dls_attempt); - /* we really want to test that it's equal to time(NULL) + delay0, but that's - * an unrealiable test, because time(NULL) might change. */ - tt_assert(download_status_get_next_attempt_at(&dls_attempt) - >= current_time + delay0); - tt_assert(download_status_get_next_attempt_at(&dls_attempt) - != TIME_MAX); - tt_int_op(download_status_get_n_failures(&dls_attempt), OP_EQ, 0); - tt_int_op(download_status_get_n_attempts(&dls_attempt), OP_EQ, 0); - tt_int_op(mock_get_options_calls, OP_GE, 1); - - /* avoid timing inconsistencies */ - dls_attempt.next_attempt_at = current_time + delay0; - - /* check that a reset schedule becomes ready at the right time */ - tt_int_op(download_status_is_ready(&dls_attempt, - current_time + delay0 - 1, 1), - OP_EQ, 0); - tt_int_op(download_status_is_ready(&dls_attempt, - current_time + delay0, 1), - OP_EQ, 1); - tt_int_op(download_status_is_ready(&dls_attempt, - current_time + delay0 + 1, 1), - OP_EQ, 1); - - /* Check that an attempt increment works */ - mock_get_options_calls = 0; - next_at = download_status_increment_attempt(&dls_attempt, "test", - current_time); - tt_assert(next_at == current_time + delay1); - tt_int_op(download_status_get_n_failures(&dls_attempt), OP_EQ, 0); - tt_int_op(download_status_get_n_attempts(&dls_attempt), OP_EQ, 1); - tt_int_op(mock_get_options_calls, OP_GE, 1); - - /* check that an incremented schedule becomes ready at the right time */ - tt_int_op(download_status_is_ready(&dls_attempt, - current_time + delay1 - 1, 1), - OP_EQ, 0); - tt_int_op(download_status_is_ready(&dls_attempt, - current_time + delay1, 1), - OP_EQ, 1); - tt_int_op(download_status_is_ready(&dls_attempt, - current_time + delay1 + 1, 1), - OP_EQ, 1); - - /* check that a schedule isn't ready if it's had too many attempts */ - tt_int_op(download_status_is_ready(&dls_attempt, - current_time + delay1 + 10, 0), - OP_EQ, 0); - - /* Check what happens when we reach then run off the end of the schedule */ - mock_get_options_calls = 0; - next_at = download_status_increment_attempt(&dls_attempt, "test", - current_time); - tt_assert(next_at == current_time + delay2); - tt_int_op(download_status_get_n_failures(&dls_attempt), OP_EQ, 0); - tt_int_op(download_status_get_n_attempts(&dls_attempt), OP_EQ, 2); - tt_int_op(mock_get_options_calls, OP_GE, 1); - - mock_get_options_calls = 0; - next_at = download_status_increment_attempt(&dls_attempt, "test", - current_time); - tt_assert(next_at == current_time + delay2); - tt_int_op(download_status_get_n_failures(&dls_attempt), OP_EQ, 0); - tt_int_op(download_status_get_n_attempts(&dls_attempt), OP_EQ, 3); - tt_int_op(mock_get_options_calls, OP_GE, 1); - - /* Check what happens when we hit the attempt limit */ - mock_get_options_calls = 0; - download_status_mark_impossible(&dls_attempt); - next_at = download_status_increment_attempt(&dls_attempt, "test", - current_time); - tt_assert(next_at == TIME_MAX); - tt_int_op(download_status_get_n_failures(&dls_attempt), OP_EQ, - IMPOSSIBLE_TO_DOWNLOAD); - tt_int_op(download_status_get_n_attempts(&dls_attempt), OP_EQ, - IMPOSSIBLE_TO_DOWNLOAD); - tt_int_op(mock_get_options_calls, OP_GE, 1); - - /* Check that an attempt reset doesn't reset at the limit */ - mock_get_options_calls = 0; - download_status_reset(&dls_attempt); - tt_assert(download_status_get_next_attempt_at(&dls_attempt) - == TIME_MAX); - tt_int_op(download_status_get_n_failures(&dls_attempt), OP_EQ, - IMPOSSIBLE_TO_DOWNLOAD); - tt_int_op(download_status_get_n_attempts(&dls_attempt), OP_EQ, - IMPOSSIBLE_TO_DOWNLOAD); - tt_int_op(mock_get_options_calls, OP_EQ, 0); - - /* Check that an attempt reset resets just before the limit */ - mock_get_options_calls = 0; - dls_attempt.n_download_failures = IMPOSSIBLE_TO_DOWNLOAD - 1; - dls_attempt.n_download_attempts = IMPOSSIBLE_TO_DOWNLOAD - 1; - download_status_reset(&dls_attempt); - /* we really want to test that it's equal to time(NULL) + delay0, but that's - * an unrealiable test, because time(NULL) might change. */ - tt_assert(download_status_get_next_attempt_at(&dls_attempt) - >= current_time + delay0); - tt_assert(download_status_get_next_attempt_at(&dls_attempt) - != TIME_MAX); - tt_int_op(download_status_get_n_failures(&dls_attempt), OP_EQ, 0); - tt_int_op(download_status_get_n_attempts(&dls_attempt), OP_EQ, 0); - tt_int_op(mock_get_options_calls, OP_GE, 1); - mock_options->UseBridges = 0; - - /* Check that attempt increments don't happen on failure-based schedules, - * and that the attempt is set at the end of time */ - mock_get_options_calls = 0; - setup_full_capture_of_logs(LOG_WARN); - next_at = download_status_increment_attempt(&dls_failure, "test", - current_time); - expect_single_log_msg_containing( - "Tried to launch an attempt-based connection on a failure-based " - "schedule."); - teardown_capture_of_logs(); - tt_assert(next_at == TIME_MAX); - tt_int_op(download_status_get_n_failures(&dls_failure), OP_EQ, 0); - tt_int_op(download_status_get_n_attempts(&dls_failure), OP_EQ, 0); - tt_int_op(mock_get_options_calls, OP_EQ, 0); - done: /* the pointers in schedule are allocated on the stack */ smartlist_free(schedule); @@ -6318,7 +5883,6 @@ struct testcase_t dir_tests[] = { DIR(post_parsing, 0), DIR(fetch_type, 0), DIR(packages, 0), - DIR(download_status_schedule, 0), DIR(download_status_random_backoff, 0), DIR(download_status_random_backoff_ranges, 0), DIR(download_status_increment, TT_FORK),

1 0

[tor/maint-0.3.3] fixup! Remove the old ("deterministic") download schedule.
by nickm＠torproject.org 03 Mar '18

03 Mar '18

commit cd4fd9887b335e83980b8d41ef223cfb651fdc2a Author: Nick Mathewson <nickm(a)torproject.org> Date: Thu Jan 25 16:05:09 2018 -0500 fixup! Remove the old ("deterministic") download schedule. oops, fix the bug number. --- changes/refactor23814 | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changes/refactor23814 b/changes/refactor23814 index 75d84bfd1..a67b6989f 100644 --- a/changes/refactor23814 +++ b/changes/refactor23814 @@ -1,4 +1,4 @@ o Code simplification and refactoring: - Remove the old (deterministic) directory retry logic entirely: We've used exponential backoff exclusively for some time. - Closes ticket 24814 + Closes ticket 23814.

1 0

[tor/maint-0.3.3] Obsolete the now-unused MaxTries options.
by nickm＠torproject.org 03 Mar '18

03 Mar '18

commit 946ebd8419fc1a2c2a95c1635ff6991119691380 Author: Nick Mathewson <nickm(a)torproject.org> Date: Wed Jan 31 15:11:47 2018 -0500 Obsolete the now-unused MaxTries options. --- src/or/config.c | 70 +++++-------------------------------------------- src/or/or.h | 26 ------------------ src/test/test_options.c | 19 -------------- 3 files changed, 6 insertions(+), 109 deletions(-) diff --git a/src/or/config.c b/src/or/config.c index afaf86785..41c944cf6 100644 --- a/src/or/config.c +++ b/src/or/config.c @@ -630,15 +630,12 @@ static config_var_t option_vars_[] = { "0, 30, 90, 600, 3600, 10800, 25200, 54000, 111600, 262800"), V(TestingClientMaxIntervalWithoutRequest, INTERVAL, "10 minutes"), V(TestingDirConnectionMaxStall, INTERVAL, "5 minutes"), - V(TestingConsensusMaxDownloadTries, UINT, "8"), - /* Since we try connections rapidly and simultaneously, we can afford - * to give up earlier. (This protects against overloading directories.) */ - V(ClientBootstrapConsensusMaxDownloadTries, UINT, "7"), - /* We want to give up much earlier if we're only using authorities. */ - V(ClientBootstrapConsensusAuthorityOnlyMaxDownloadTries, UINT, "4"), - V(TestingDescriptorMaxDownloadTries, UINT, "8"), - V(TestingMicrodescMaxDownloadTries, UINT, "8"), - V(TestingCertMaxDownloadTries, UINT, "8"), + OBSOLETE("TestingConsensusMaxDownloadTries"), + OBSOLETE("ClientBootstrapConsensusMaxDownloadTries"), + OBSOLETE("ClientBootstrapConsensusAuthorityOnlyMaxDownloadTries"), + OBSOLETE("TestingDescriptorMaxDownloadTries"), + OBSOLETE("TestingMicrodescMaxDownloadTries"), + OBSOLETE("TestingCertMaxDownloadTries"), V(TestingDirAuthVoteExit, ROUTERSET, NULL), V(TestingDirAuthVoteExitIsStrict, BOOL, "0"), V(TestingDirAuthVoteGuard, ROUTERSET, NULL), @@ -663,8 +660,6 @@ static const config_var_t testing_tor_network_defaults[] = { "0, 1, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 8, 16, 32, 60"), V(ClientBootstrapConsensusAuthorityOnlyDownloadSchedule, CSV_INTERVAL, "0, 1, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 8, 16, 32, 60"), - V(ClientBootstrapConsensusMaxDownloadTries, UINT, "80"), - V(ClientBootstrapConsensusAuthorityOnlyMaxDownloadTries, UINT, "80"), V(ClientDNSRejectInternalAddresses, BOOL,"0"), V(ClientRejectInternalAddresses, BOOL, "0"), V(CountPrivateBandwidth, BOOL, "1"), @@ -692,10 +687,6 @@ static const config_var_t testing_tor_network_defaults[] = { "15, 20, 30, 60"), V(TestingClientMaxIntervalWithoutRequest, INTERVAL, "5 seconds"), V(TestingDirConnectionMaxStall, INTERVAL, "30 seconds"), - V(TestingConsensusMaxDownloadTries, UINT, "80"), - V(TestingDescriptorMaxDownloadTries, UINT, "80"), - V(TestingMicrodescMaxDownloadTries, UINT, "80"), - V(TestingCertMaxDownloadTries, UINT, "80"), V(TestingEnableConnBwEvent, BOOL, "1"), V(TestingEnableCellStatsEvent, BOOL, "1"), V(TestingEnableTbEmptyEvent, BOOL, "1"), @@ -4359,10 +4350,6 @@ options_validate(or_options_t *old_options, or_options_t *options, CHECK_DEFAULT(TestingBridgeBootstrapDownloadSchedule); CHECK_DEFAULT(TestingClientMaxIntervalWithoutRequest); CHECK_DEFAULT(TestingDirConnectionMaxStall); - CHECK_DEFAULT(TestingConsensusMaxDownloadTries); - CHECK_DEFAULT(TestingDescriptorMaxDownloadTries); - CHECK_DEFAULT(TestingMicrodescMaxDownloadTries); - CHECK_DEFAULT(TestingCertMaxDownloadTries); CHECK_DEFAULT(TestingAuthKeyLifetime); CHECK_DEFAULT(TestingLinkCertLifetime); CHECK_DEFAULT(TestingSigningKeySlop); @@ -4437,33 +4424,6 @@ options_validate(or_options_t *old_options, or_options_t *options, COMPLAIN("TestingDirConnectionMaxStall is insanely high."); } - if (options->TestingConsensusMaxDownloadTries < 2) { - REJECT("TestingConsensusMaxDownloadTries must be greater than 2."); - } else if (options->TestingConsensusMaxDownloadTries > 800) { - COMPLAIN("TestingConsensusMaxDownloadTries is insanely high."); - } - - if (options->ClientBootstrapConsensusMaxDownloadTries < 2) { - REJECT("ClientBootstrapConsensusMaxDownloadTries must be greater " - "than 2." - ); - } else if (options->ClientBootstrapConsensusMaxDownloadTries > 800) { - COMPLAIN("ClientBootstrapConsensusMaxDownloadTries is insanely " - "high."); - } - - if (options->ClientBootstrapConsensusAuthorityOnlyMaxDownloadTries - < 2) { - REJECT("ClientBootstrapConsensusAuthorityOnlyMaxDownloadTries must " - "be greater than 2." - ); - } else if ( - options->ClientBootstrapConsensusAuthorityOnlyMaxDownloadTries - > 800) { - COMPLAIN("ClientBootstrapConsensusAuthorityOnlyMaxDownloadTries is " - "insanely high."); - } - if (options->ClientBootstrapConsensusMaxInProgressTries < 1) { REJECT("ClientBootstrapConsensusMaxInProgressTries must be greater " "than 0."); @@ -4473,24 +4433,6 @@ options_validate(or_options_t *old_options, or_options_t *options, "high."); } - if (options->TestingDescriptorMaxDownloadTries < 2) { - REJECT("TestingDescriptorMaxDownloadTries must be greater than 1."); - } else if (options->TestingDescriptorMaxDownloadTries > 800) { - COMPLAIN("TestingDescriptorMaxDownloadTries is insanely high."); - } - - if (options->TestingMicrodescMaxDownloadTries < 2) { - REJECT("TestingMicrodescMaxDownloadTries must be greater than 1."); - } else if (options->TestingMicrodescMaxDownloadTries > 800) { - COMPLAIN("TestingMicrodescMaxDownloadTries is insanely high."); - } - - if (options->TestingCertMaxDownloadTries < 2) { - REJECT("TestingCertMaxDownloadTries must be greater than 1."); - } else if (options->TestingCertMaxDownloadTries > 800) { - COMPLAIN("TestingCertMaxDownloadTries is insanely high."); - } - if (options->TestingEnableConnBwEvent && !options->TestingTorNetwork && !options->UsingTestNetworkDefaults_) { REJECT("TestingEnableConnBwEvent may only be changed in testing " diff --git a/src/or/or.h b/src/or/or.h index f93050dfe..c51a2137e 100644 --- a/src/or/or.h +++ b/src/or/or.h @@ -4397,37 +4397,11 @@ typedef struct { * it? Only altered on testing networks. */ int TestingDirConnectionMaxStall; - /** How many times will we try to fetch a consensus before we give - * up? Only altered on testing networks. */ - int TestingConsensusMaxDownloadTries; - - /** How many times will a client try to fetch a consensus while - * bootstrapping using a list of fallback directories, before it gives up? - * Only altered on testing networks. */ - int ClientBootstrapConsensusMaxDownloadTries; - - /** How many times will a client try to fetch a consensus while - * bootstrapping using only a list of authorities, before it gives up? - * Only altered on testing networks. */ - int ClientBootstrapConsensusAuthorityOnlyMaxDownloadTries; - /** How many simultaneous in-progress connections will we make when trying * to fetch a consensus before we wait for one to complete, timeout, or * error out? Only altered on testing networks. */ int ClientBootstrapConsensusMaxInProgressTries; - /** How many times will we try to download a router's descriptor before - * giving up? Only altered on testing networks. */ - int TestingDescriptorMaxDownloadTries; - - /** How many times will we try to download a microdescriptor before - * giving up? Only altered on testing networks. */ - int TestingMicrodescMaxDownloadTries; - - /** How many times will we try to fetch a certificate before giving - * up? Only altered on testing networks. */ - int TestingCertMaxDownloadTries; - /** If true, we take part in a testing network. Change the defaults of a * couple of other configuration options and allow to change the values * of certain configuration options. */ diff --git a/src/test/test_options.c b/src/test/test_options.c index 62732cabf..1ffc029cb 100644 --- a/src/test/test_options.c +++ b/src/test/test_options.c @@ -377,17 +377,11 @@ fixed_get_uname(void) } #define TEST_OPTIONS_OLD_VALUES "TestingV3AuthInitialVotingInterval 1800\n" \ - "ClientBootstrapConsensusMaxDownloadTries 7\n" \ - "ClientBootstrapConsensusAuthorityOnlyMaxDownloadTries 4\n" \ "ClientBootstrapConsensusMaxInProgressTries 3\n" \ "TestingV3AuthInitialVoteDelay 300\n" \ "TestingV3AuthInitialDistDelay 300\n" \ "TestingClientMaxIntervalWithoutRequest 600\n" \ "TestingDirConnectionMaxStall 600\n" \ - "TestingConsensusMaxDownloadTries 8\n" \ - "TestingDescriptorMaxDownloadTries 8\n" \ - "TestingMicrodescMaxDownloadTries 8\n" \ - "TestingCertMaxDownloadTries 8\n" #define TEST_OPTIONS_DEFAULT_VALUES TEST_OPTIONS_OLD_VALUES \ "MaxClientCircuitsPending 1\n" \ @@ -2081,10 +2075,6 @@ test_options_validate__testing(void *ignored) ENSURE_DEFAULT(TestingBridgeBootstrapDownloadSchedule, 3000); ENSURE_DEFAULT(TestingClientMaxIntervalWithoutRequest, 3000); ENSURE_DEFAULT(TestingDirConnectionMaxStall, 3000); - ENSURE_DEFAULT(TestingConsensusMaxDownloadTries, 3000); - ENSURE_DEFAULT(TestingDescriptorMaxDownloadTries, 3000); - ENSURE_DEFAULT(TestingMicrodescMaxDownloadTries, 3000); - ENSURE_DEFAULT(TestingCertMaxDownloadTries, 3000); ENSURE_DEFAULT(TestingAuthKeyLifetime, 3000); ENSURE_DEFAULT(TestingLinkCertLifetime, 3000); ENSURE_DEFAULT(TestingSigningKeySlop, 3000); @@ -4069,15 +4059,6 @@ test_options_validate__testing_options(void *ignored) "is way too low."); TEST_TESTING_OPTION(TestingDirConnectionMaxStall, 1, 3601, "is way too low."); - // TODO: I think this points to a bug/regression in options_validate - TEST_TESTING_OPTION(TestingConsensusMaxDownloadTries, 1, 801, - "must be greater than 2."); - TEST_TESTING_OPTION(TestingDescriptorMaxDownloadTries, 1, 801, - "must be greater than 1."); - TEST_TESTING_OPTION(TestingMicrodescMaxDownloadTries, 1, 801, - "must be greater than 1."); - TEST_TESTING_OPTION(TestingCertMaxDownloadTries, 1, 801, - "must be greater than 1."); free_options_test_data(tdata); tdata = get_options_test_data(TEST_OPTIONS_DEFAULT_VALUES

1 0

[tor/maint-0.3.3] Merge branch 'ticket23814' into maint-0.3.3
by nickm＠torproject.org 03 Mar '18

03 Mar '18

commit cc7de9ce1d0532d97623a74ed014e39c62a6a840 Merge: aec505a31 30225fd89 Author: Nick Mathewson <nickm(a)torproject.org> Date: Sat Mar 3 11:53:01 2018 -0500 Merge branch 'ticket23814' into maint-0.3.3 changes/refactor23814 | 4 + doc/tor.1.txt | 30 --- src/or/bridges.c | 4 +- src/or/config.c | 70 +------ src/or/control.c | 39 +--- src/or/directory.c | 122 ++++-------- src/or/directory.h | 25 +-- src/or/microdesc.c | 3 +- src/or/networkstatus.c | 27 +-- src/or/or.h | 40 ---- src/or/routerlist.c | 19 +- src/or/routerparse.c | 1 - src/test/test_controller.c | 26 ++- src/test/test_dir.c | 467 ++------------------------------------------- src/test/test_options.c | 19 -- 15 files changed, 109 insertions(+), 787 deletions(-)

1 0

[tor/maint-0.3.3] Remove MaxDownloadTries options from the manpage
by nickm＠torproject.org 03 Mar '18

03 Mar '18

commit 30225fd89a1899cad4f53506773939ca64ead8b0 Author: Nick Mathewson <nickm(a)torproject.org> Date: Wed Jan 31 15:14:48 2018 -0500 Remove MaxDownloadTries options from the manpage --- doc/tor.1.txt | 30 ------------------------------ 1 file changed, 30 deletions(-) diff --git a/doc/tor.1.txt b/doc/tor.1.txt index ef3d1eb9e..024ef1e15 100644 --- a/doc/tor.1.txt +++ b/doc/tor.1.txt @@ -1757,14 +1757,6 @@ The following options are useful only for clients (that is, if which are advanced by connection failures. (Default: 0, 3, 7, 3600, 10800, 25200, 54000, 111600, 262800) -[[ClientBootstrapConsensusMaxDownloadTries]] **ClientBootstrapConsensusMaxDownloadTries** __NUM__:: - Try this many times to download a consensus while bootstrapping using - fallback directory mirrors before giving up. (Default: 7) - -[[ClientBootstrapConsensusAuthorityOnlyMaxDownloadTries]] **ClientBootstrapConsensusAuthorityOnlyMaxDownloadTries** __NUM__:: - Try this many times to download a consensus while bootstrapping using - authorities before giving up. (Default: 4) - [[ClientBootstrapConsensusMaxInProgressTries]] **ClientBootstrapConsensusMaxInProgressTries** __NUM__:: Try this many simultaneous connections to download a consensus before waiting for one to complete, timeout, or error out. (Default: 3) @@ -2776,8 +2768,6 @@ The following options are used for running a testing Tor network. 4 (for 40 seconds), 8, 16, 32, 60 ClientBootstrapConsensusAuthorityOnlyDownloadSchedule 0, 1, 4 (for 40 seconds), 8, 16, 32, 60 - ClientBootstrapConsensusMaxDownloadTries 80 - ClientBootstrapConsensusAuthorityOnlyMaxDownloadTries 80 ClientDNSRejectInternalAddresses 0 ClientRejectInternalAddresses 0 CountPrivateBandwidth 1 @@ -2800,10 +2790,6 @@ The following options are used for running a testing Tor network. TestingBridgeBootstrapDownloadSchedule 0, 0, 5, 10, 15, 20, 30, 60 TestingClientMaxIntervalWithoutRequest 5 seconds TestingDirConnectionMaxStall 30 seconds - TestingConsensusMaxDownloadTries 80 - TestingDescriptorMaxDownloadTries 80 - TestingMicrodescMaxDownloadTries 80 - TestingCertMaxDownloadTries 80 TestingEnableConnBwEvent 1 TestingEnableCellStatsEvent 1 TestingEnableTbEmptyEvent 1 @@ -2884,22 +2870,6 @@ The following options are used for running a testing Tor network. Changing this requires that **TestingTorNetwork** is set. (Default: 5 minutes) -[[TestingConsensusMaxDownloadTries]] **TestingConsensusMaxDownloadTries** __NUM__:: - Try this many times to download a consensus before giving up. Changing - this requires that **TestingTorNetwork** is set. (Default: 8) - -[[TestingDescriptorMaxDownloadTries]] **TestingDescriptorMaxDownloadTries** __NUM__:: - Try this often to download a server descriptor before giving up. - Changing this requires that **TestingTorNetwork** is set. (Default: 8) - -[[TestingMicrodescMaxDownloadTries]] **TestingMicrodescMaxDownloadTries** __NUM__:: - Try this often to download a microdesc descriptor before giving up. - Changing this requires that **TestingTorNetwork** is set. (Default: 8) - -[[TestingCertMaxDownloadTries]] **TestingCertMaxDownloadTries** __NUM__:: - Try this often to download a v3 authority certificate before giving up. - Changing this requires that **TestingTorNetwork** is set. (Default: 8) - [[TestingDirAuthVoteExit]] **TestingDirAuthVoteExit** __node__,__node__,__...__:: A list of identity fingerprints, country codes, and address patterns of nodes to vote Exit for regardless of their

1 0

[tor/maint-0.3.3] Remove two vestigial MaxDownloadTries checks from directory.c
by nickm＠torproject.org 03 Mar '18

03 Mar '18

commit c0024edd2651761a2740d422a9dceca65c3721cf Author: Nick Mathewson <nickm(a)torproject.org> Date: Wed Jan 31 15:08:46 2018 -0500 Remove two vestigial MaxDownloadTries checks from directory.c These are no longer meaningful, since there's no longer an upper limit to how many times (in the exponential-backoff world) one can retry a download. download_status_is_ready() didn't check these any more, and neither do we. --- src/or/directory.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/src/or/directory.c b/src/or/directory.c index 3dbed28e7..34a34d9ba 100644 --- a/src/or/directory.c +++ b/src/or/directory.c @@ -5730,8 +5730,7 @@ dir_routerdesc_download_failed(smartlist_t *failed, int status_code, } else { dls = router_get_dl_status_by_descriptor_digest(digest); } - if (!dls || dls->n_download_failures >= - get_options()->TestingDescriptorMaxDownloadTries) + if (!dls) continue; download_status_increment_failure(dls, status_code, cp, server, now); } SMARTLIST_FOREACH_END(cp); @@ -5768,10 +5767,6 @@ dir_microdesc_download_failed(smartlist_t *failed, if (!rs) continue; dls = &rs->dl_status; - if (dls->n_download_failures >= - get_options()->TestingMicrodescMaxDownloadTries) { - continue; - } { /* Increment the failure count for this md fetch */ char buf[BASE64_DIGEST256_LEN+1];

1 0

[tor/master] remove the max_failures argument from download_status_is_ready.
by nickm＠torproject.org 03 Mar '18

03 Mar '18

commit b8ff7407a71fc51193dedb8cf952805c0fb3e774 Author: Nick Mathewson <nickm(a)torproject.org> Date: Wed Jan 31 15:03:47 2018 -0500 remove the max_failures argument from download_status_is_ready. --- src/or/bridges.c | 3 +-- src/or/directory.h | 8 ++------ src/or/microdesc.c | 3 +-- src/or/networkstatus.c | 19 ++++--------------- src/or/routerlist.c | 18 +++++++----------- 5 files changed, 15 insertions(+), 36 deletions(-) diff --git a/src/or/bridges.c b/src/or/bridges.c index 3d9af21fa..6cd208473 100644 --- a/src/or/bridges.c +++ b/src/or/bridges.c @@ -636,8 +636,7 @@ fetch_bridge_descriptors(const or_options_t *options, time_t now) SMARTLIST_FOREACH_BEGIN(bridge_list, bridge_info_t *, bridge) { /* This resets the download status on first use */ - if (!download_status_is_ready(&bridge->fetch_status, now, - IMPOSSIBLE_TO_DOWNLOAD)) + if (!download_status_is_ready(&bridge->fetch_status, now)) continue; /* don't bother, no need to retry yet */ if (routerset_contains_bridge(options->ExcludeNodes, bridge)) { download_status_mark_impossible(&bridge->fetch_status); diff --git a/src/or/directory.h b/src/or/directory.h index e223d51ae..aa4d29a5b 100644 --- a/src/or/directory.h +++ b/src/or/directory.h @@ -132,18 +132,14 @@ time_t download_status_increment_attempt(download_status_t *dls, time(NULL)) void download_status_reset(download_status_t *dls); -static int download_status_is_ready(download_status_t *dls, time_t now, - int max_failures); +static int download_status_is_ready(download_status_t *dls, time_t now); time_t download_status_get_next_attempt_at(const download_status_t *dls); /** Return true iff, as of now, the resource tracked by dls is * ready to get its download reattempted. */ static inline int -download_status_is_ready(download_status_t *dls, time_t now, - int max_failures) +download_status_is_ready(download_status_t *dls, time_t now) { - (void) max_failures; // 23814 REMOVE - /* dls wasn't reset before it was used */ if (dls->next_attempt_at == 0) { download_status_reset(dls); diff --git a/src/or/microdesc.c b/src/or/microdesc.c index d8a4660af..b4a934e09 100644 --- a/src/or/microdesc.c +++ b/src/or/microdesc.c @@ -920,8 +920,7 @@ microdesc_list_missing_digest256(networkstatus_t *ns, microdesc_cache_t *cache, if (microdesc_cache_lookup_by_digest256(cache, rs->descriptor_digest)) continue; if (downloadable_only && - !download_status_is_ready(&rs->dl_status, now, - get_options()->TestingMicrodescMaxDownloadTries)) + !download_status_is_ready(&rs->dl_status, now)) continue; if (skip && digest256map_get(skip, (const uint8_t*)rs->descriptor_digest)) continue; diff --git a/src/or/networkstatus.c b/src/or/networkstatus.c index e051e41f5..b9c6c5ba9 100644 --- a/src/or/networkstatus.c +++ b/src/or/networkstatus.c @@ -942,14 +942,11 @@ update_consensus_networkstatus_downloads(time_t now) update_consensus_bootstrap_multiple_downloads(now, options); } else { /* Check if we failed downloading a consensus too recently */ - int max_dl_tries = options->TestingConsensusMaxDownloadTries; /* Let's make sure we remembered to update consensus_dl_status */ tor_assert(consensus_dl_status[i].schedule == DL_SCHED_CONSENSUS); - if (!download_status_is_ready(&consensus_dl_status[i], - now, - max_dl_tries)) { + if (!download_status_is_ready(&consensus_dl_status[i], now)) { continue; } @@ -976,17 +973,9 @@ update_consensus_networkstatus_downloads(time_t now) static void update_consensus_bootstrap_attempt_downloads( time_t now, - const or_options_t *options, download_status_t *dls, download_want_authority_t want_authority) { - int use_fallbacks = networkstatus_consensus_can_use_extra_fallbacks(options); - int max_dl_tries = options->ClientBootstrapConsensusMaxDownloadTries; - if (!use_fallbacks) { - max_dl_tries = - options->ClientBootstrapConsensusAuthorityOnlyMaxDownloadTries; - } - const char *resource = networkstatus_get_flavor_name( usable_consensus_flavor()); @@ -995,7 +984,7 @@ update_consensus_bootstrap_attempt_downloads( /* Allow for multiple connections in the same second, if the schedule value * is 0. */ - while (download_status_is_ready(dls, now, max_dl_tries)) { + while (download_status_is_ready(dls, now)) { log_info(LD_DIR, "Launching %s bootstrap %s networkstatus consensus " "download.", resource, (want_authority == DL_WANT_AUTHORITY ? "authority" @@ -1046,7 +1035,7 @@ update_consensus_bootstrap_multiple_downloads(time_t now, if (!check_consensus_waiting_for_certs(usable_flavor, now, dls_f)) { /* During bootstrap, DL_WANT_ANY_DIRSERVER means "use fallbacks". */ - update_consensus_bootstrap_attempt_downloads(now, options, dls_f, + update_consensus_bootstrap_attempt_downloads(now, dls_f, DL_WANT_ANY_DIRSERVER); } } @@ -1056,7 +1045,7 @@ update_consensus_bootstrap_multiple_downloads(time_t now, &consensus_bootstrap_dl_status[CONSENSUS_BOOTSTRAP_SOURCE_AUTHORITY]; if (!check_consensus_waiting_for_certs(usable_flavor, now, dls_a)) { - update_consensus_bootstrap_attempt_downloads(now, options, dls_a, + update_consensus_bootstrap_attempt_downloads(now, dls_a, DL_WANT_AUTHORITY); } } diff --git a/src/or/routerlist.c b/src/or/routerlist.c index fec224fad..18208a7a0 100644 --- a/src/or/routerlist.c +++ b/src/or/routerlist.c @@ -177,7 +177,7 @@ static void download_status_reset_by_sk_in_cl(cert_list_t *cl, const char *digest); static int download_status_is_ready_by_sk_in_cl(cert_list_t *cl, const char *digest, - time_t now, int max_failures); + time_t now); /****************************************************************************/ @@ -287,7 +287,7 @@ download_status_reset_by_sk_in_cl(cert_list_t *cl, const char *digest) static int download_status_is_ready_by_sk_in_cl(cert_list_t *cl, const char *digest, - time_t now, int max_failures) + time_t now) { int rv = 0; download_status_t *dlstatus = NULL; @@ -304,7 +304,7 @@ download_status_is_ready_by_sk_in_cl(cert_list_t *cl, /* Got one? */ if (dlstatus) { /* Use download_status_is_ready() */ - rv = download_status_is_ready(dlstatus, now, max_failures); + rv = download_status_is_ready(dlstatus, now); } else { /* * If we don't know anything about it, return 1, since we haven't @@ -1067,8 +1067,7 @@ authority_certs_fetch_missing(networkstatus_t *status, time_t now, } } SMARTLIST_FOREACH_END(cert); if (!found && - download_status_is_ready(&(cl->dl_status_by_id), now, - options->TestingCertMaxDownloadTries) && + download_status_is_ready(&(cl->dl_status_by_id), now) && !digestmap_get(pending_id, ds->v3_identity_digest)) { log_info(LD_DIR, "No current certificate known for authority %s " @@ -1131,8 +1130,7 @@ authority_certs_fetch_missing(networkstatus_t *status, time_t now, continue; } if (download_status_is_ready_by_sk_in_cl( - cl, sig->signing_key_digest, - now, options->TestingCertMaxDownloadTries) && + cl, sig->signing_key_digest, now) && !fp_pair_map_get_by_digests(pending_cert, voter->identity_digest, sig->signing_key_digest)) { @@ -5166,8 +5164,7 @@ update_consensus_router_descriptor_downloads(time_t now, int is_vote, ++n_inprogress; continue; /* We have an in-progress download. */ } - if (!download_status_is_ready(&rs->dl_status, now, - options->TestingDescriptorMaxDownloadTries)) { + if (!download_status_is_ready(&rs->dl_status, now)) { ++n_delayed; /* Not ready for retry. */ continue; } @@ -5343,8 +5340,7 @@ update_extrainfo_downloads(time_t now) ++n_have; continue; } - if (!download_status_is_ready(&sd->ei_dl_status, now, - options->TestingDescriptorMaxDownloadTries)) { + if (!download_status_is_ready(&sd->ei_dl_status, now)) { ++n_delay; continue; }

1 0

[tor/maint-0.3.3] Remove all the old max_delay logic.
by nickm＠torproject.org 03 Mar '18

03 Mar '18

commit 5b55e15707fd49079b9c127b339976c4f446e131 Author: Nick Mathewson <nickm(a)torproject.org> Date: Thu Jan 25 16:05:20 2018 -0500 Remove all the old max_delay logic. We had tests for it, but it was always INT_MAX. --- src/or/directory.c | 53 +++++++++++++++++------------------------------------ src/or/directory.h | 10 ++++------ src/test/test_dir.c | 27 ++++++++++----------------- 3 files changed, 31 insertions(+), 59 deletions(-) diff --git a/src/or/directory.c b/src/or/directory.c index 3d5321157..3dbed28e7 100644 --- a/src/or/directory.c +++ b/src/or/directory.c @@ -5362,17 +5362,14 @@ find_dl_schedule(const download_status_t *dls, const or_options_t *options) return NULL; } -/** Decide which minimum and maximum delay step we want to use based on +/** Decide which minimum delay step we want to use based on * descriptor type in dls and options. * Helper function for download_status_schedule_get_delay(). */ -STATIC void -find_dl_min_and_max_delay(download_status_t *dls, const or_options_t *options, - int *min, int *max) +STATIC int +find_dl_min_delay(download_status_t *dls, const or_options_t *options) { tor_assert(dls); tor_assert(options); - tor_assert(min); - tor_assert(max); /* * For now, just use the existing schedule config stuff and pick the @@ -5380,10 +5377,7 @@ find_dl_min_and_max_delay(download_status_t *dls, const or_options_t *options, */ const smartlist_t *schedule = find_dl_schedule(dls, options); tor_assert(schedule != NULL && smartlist_len(schedule) >= 2); - *min = *((int *)(smartlist_get(schedule, 0))); - /* Increment on failure schedules always use exponential backoff, but they - * have a smaller limit when they're deterministic */ - *max = INT_MAX; + return *(int *)(smartlist_get(schedule, 0)); } /** As next_random_exponential_delay() below, but does not compute a random @@ -5421,49 +5415,39 @@ next_random_exponential_delay_range(int *low_bound_out, * zero); the backoff_position parameter is the number of times we've * generated a delay; and the delay argument is the most recently used * delay. - * - * Requires that delay is less than INT_MAX, and delay is in [0,max_delay]. */ STATIC int next_random_exponential_delay(int delay, - int base_delay, - int max_delay) + int base_delay) { /* Check preconditions */ - if (BUG(max_delay < 0)) - max_delay = 0; - if (BUG(delay > max_delay)) - delay = max_delay; if (BUG(delay < 0)) delay = 0; if (base_delay < 1) base_delay = 1; - int low_bound=0, high_bound=max_delay; + int low_bound=0, high_bound=INT_MAX; next_random_exponential_delay_range(&low_bound, &high_bound, delay, base_delay); - int rand_delay = crypto_rand_int_range(low_bound, high_bound); - - return MIN(rand_delay, max_delay); + return crypto_rand_int_range(low_bound, high_bound); } -/** Find the current delay for dls based on min_delay/ - * max_delay. Requires that 0 <= min_delay <= max_delay <= INT_MAX. +/** Find the current delay for dls based on min_delay. * * This function sets dls->next_attempt_at based on now, and returns the delay. * Helper for download_status_increment_failure and * download_status_increment_attempt. */ STATIC int download_status_schedule_get_delay(download_status_t *dls, - int min_delay, int max_delay, + int min_delay, time_t now) { tor_assert(dls); /* If we're using random exponential backoff, we do need min/max delay */ - tor_assert(min_delay >= 0 && max_delay >= min_delay); + tor_assert(min_delay >= 0); int delay = INT_MAX; uint8_t dls_schedule_position = (dls->increment_on @@ -5482,7 +5466,7 @@ download_status_schedule_get_delay(download_status_t *dls, while (dls->last_backoff_position < dls_schedule_position) { /* Do one increment step */ - delay = next_random_exponential_delay(delay, min_delay, max_delay); + delay = next_random_exponential_delay(delay, min_delay); /* Update our position */ ++(dls->last_backoff_position); } @@ -5493,7 +5477,6 @@ download_status_schedule_get_delay(download_status_t *dls, /* Clamp it within min/max if we have them */ if (min_delay >= 0 && delay < min_delay) delay = min_delay; - if (max_delay != INT_MAX && delay > max_delay) delay = max_delay; /* Store it for next time */ dls->last_backoff_position = dls_schedule_position; @@ -5560,7 +5543,7 @@ download_status_increment_failure(download_status_t *dls, int status_code, (void) status_code; // XXXX no longer used. (void) server; // XXXX no longer used. int increment = -1; - int min_delay = 0, max_delay = INT_MAX; + int min_delay = 0; tor_assert(dls); @@ -5585,9 +5568,8 @@ download_status_increment_failure(download_status_t *dls, int status_code, /* only return a failure retry time if this schedule increments on failures */ - find_dl_min_and_max_delay(dls, get_options(), &min_delay, &max_delay); - increment = download_status_schedule_get_delay(dls, - min_delay, max_delay, now); + min_delay = find_dl_min_delay(dls, get_options()); + increment = download_status_schedule_get_delay(dls, min_delay, now); } download_status_log_helper(item, !dls->increment_on, "failed", @@ -5618,7 +5600,7 @@ download_status_increment_attempt(download_status_t *dls, const char *item, time_t now) { int delay = -1; - int min_delay = 0, max_delay = INT_MAX; + int min_delay = 0; tor_assert(dls); @@ -5638,9 +5620,8 @@ download_status_increment_attempt(download_status_t *dls, const char *item, if (dls->n_download_attempts < IMPOSSIBLE_TO_DOWNLOAD-1) ++dls->n_download_attempts; - find_dl_min_and_max_delay(dls, get_options(), &min_delay, &max_delay); - delay = download_status_schedule_get_delay(dls, - min_delay, max_delay, now); + min_delay = find_dl_min_delay(dls, get_options()); + delay = download_status_schedule_get_delay(dls, min_delay, now); download_status_log_helper(item, dls->increment_on, "attempted", "on failure", dls->n_download_attempts, diff --git a/src/or/directory.h b/src/or/directory.h index 6008d3a88..e223d51ae 100644 --- a/src/or/directory.h +++ b/src/or/directory.h @@ -254,7 +254,7 @@ MOCK_DECL(STATIC int, directory_handle_command_post,(dir_connection_t *conn, const char *body, size_t body_len)); STATIC int download_status_schedule_get_delay(download_status_t *dls, - int min_delay, int max_delay, + int min_delay, time_t now); STATIC int handle_post_hs_descriptor(const char *url, const char *body); @@ -265,13 +265,11 @@ STATIC int should_use_directory_guards(const or_options_t *options); STATIC compression_level_t choose_compression_level(ssize_t n_bytes); STATIC const smartlist_t *find_dl_schedule(const download_status_t *dls, const or_options_t *options); -STATIC void find_dl_min_and_max_delay(download_status_t *dls, - const or_options_t *options, - int *min, int *max); +STATIC int find_dl_min_delay(download_status_t *dls, + const or_options_t *options); STATIC int next_random_exponential_delay(int delay, - int base_delay, - int max_delay); + int base_delay); STATIC void next_random_exponential_delay_range(int *low_bound_out, int *high_bound_out, diff --git a/src/test/test_dir.c b/src/test/test_dir.c index dc8d2a3bc..929ccd6c5 100644 --- a/src/test/test_dir.c +++ b/src/test/test_dir.c @@ -3965,7 +3965,7 @@ test_dir_packages(void *arg) } static void -download_status_random_backoff_helper(int min_delay, int max_delay) +download_status_random_backoff_helper(int min_delay) { download_status_t dls_random = { 0, 0, 0, DL_SCHED_GENERIC, DL_WANT_AUTHORITY, @@ -3978,22 +3978,21 @@ download_status_random_backoff_helper(int min_delay, int max_delay) int n_attempts = 0; do { increment = download_status_schedule_get_delay(&dls_random, - min_delay, max_delay, + min_delay, current_time); - log_debug(LD_DIR, "Min: %d, Max: %d, Inc: %d, Old Inc: %d", - min_delay, max_delay, increment, old_increment); + log_debug(LD_DIR, "Min: %d, Inc: %d, Old Inc: %d", + min_delay, increment, old_increment); /* Regression test for 20534 and friends * increment must always increase after the first */ - if (dls_random.last_backoff_position > 0 && max_delay > 0) { + if (dls_random.last_backoff_position > 0) { /* Always increment the exponential backoff */ tt_int_op(increment, OP_GE, 1); } /* Test */ tt_int_op(increment, OP_GE, min_delay); - tt_int_op(increment, OP_LE, max_delay); /* Advance */ if (dls_random.n_download_attempts < IMPOSSIBLE_TO_DOWNLOAD - 1) { @@ -4003,7 +4002,7 @@ download_status_random_backoff_helper(int min_delay, int max_delay) /* Try another maybe */ old_increment = increment; - } while (increment < max_delay && ++n_attempts < 1000); + } while (++n_attempts < 1000); done: return; @@ -4015,19 +4014,13 @@ test_dir_download_status_random_backoff(void *arg) (void)arg; /* Do a standard test */ - download_status_random_backoff_helper(0, 1000000); - /* Regression test for 20534 and friends: - * try tighter bounds */ - download_status_random_backoff_helper(0, 100); + download_status_random_backoff_helper(0); /* regression tests for 17750: initial delay */ - download_status_random_backoff_helper(10, 1000); - download_status_random_backoff_helper(20, 30); + download_status_random_backoff_helper(10); + download_status_random_backoff_helper(20); /* Pathological cases */ - download_status_random_backoff_helper(0, 0); - download_status_random_backoff_helper(1, 1); - download_status_random_backoff_helper(0, INT_MAX); - download_status_random_backoff_helper(INT_MAX/2, INT_MAX); + download_status_random_backoff_helper(INT_MAX/2); } static void

1 0