Remove all the old max_delay logic.

We had tests for it, but it was always INT_MAX.
This commit is contained in:
Nick Mathewson 2018-01-25 16:05:20 -05:00
parent cd4fd9887b
commit 5b55e15707
3 changed files with 31 additions and 59 deletions

View File

@ -5362,17 +5362,14 @@ find_dl_schedule(const download_status_t *dls, const or_options_t *options)
return NULL; 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 <b>dls</b> and <b>options</b>. * descriptor type in <b>dls</b> and <b>options</b>.
* Helper function for download_status_schedule_get_delay(). */ * Helper function for download_status_schedule_get_delay(). */
STATIC void STATIC int
find_dl_min_and_max_delay(download_status_t *dls, const or_options_t *options, find_dl_min_delay(download_status_t *dls, const or_options_t *options)
int *min, int *max)
{ {
tor_assert(dls); tor_assert(dls);
tor_assert(options); tor_assert(options);
tor_assert(min);
tor_assert(max);
/* /*
* For now, just use the existing schedule config stuff and pick the * 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); const smartlist_t *schedule = find_dl_schedule(dls, options);
tor_assert(schedule != NULL && smartlist_len(schedule) >= 2); tor_assert(schedule != NULL && smartlist_len(schedule) >= 2);
*min = *((int *)(smartlist_get(schedule, 0))); return *(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;
} }
/** As next_random_exponential_delay() below, but does not compute a random /** 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 <b>backoff_position</b> parameter is the number of times we've * zero); the <b>backoff_position</b> parameter is the number of times we've
* generated a delay; and the <b>delay</b> argument is the most recently used * generated a delay; and the <b>delay</b> argument is the most recently used
* delay. * delay.
*
* Requires that delay is less than INT_MAX, and delay is in [0,max_delay].
*/ */
STATIC int STATIC int
next_random_exponential_delay(int delay, next_random_exponential_delay(int delay,
int base_delay, int base_delay)
int max_delay)
{ {
/* Check preconditions */ /* Check preconditions */
if (BUG(max_delay < 0))
max_delay = 0;
if (BUG(delay > max_delay))
delay = max_delay;
if (BUG(delay < 0)) if (BUG(delay < 0))
delay = 0; delay = 0;
if (base_delay < 1) if (base_delay < 1)
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, next_random_exponential_delay_range(&low_bound, &high_bound,
delay, base_delay); delay, base_delay);
int rand_delay = crypto_rand_int_range(low_bound, high_bound); return crypto_rand_int_range(low_bound, high_bound);
return MIN(rand_delay, max_delay);
} }
/** Find the current delay for dls based on min_delay/ /** 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. * This function sets dls->next_attempt_at based on now, and returns the delay.
* Helper for download_status_increment_failure and * Helper for download_status_increment_failure and
* download_status_increment_attempt. */ * download_status_increment_attempt. */
STATIC int STATIC int
download_status_schedule_get_delay(download_status_t *dls, download_status_schedule_get_delay(download_status_t *dls,
int min_delay, int max_delay, int min_delay,
time_t now) time_t now)
{ {
tor_assert(dls); tor_assert(dls);
/* If we're using random exponential backoff, we do need min/max delay */ /* 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; int delay = INT_MAX;
uint8_t dls_schedule_position = (dls->increment_on 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) { while (dls->last_backoff_position < dls_schedule_position) {
/* Do one increment step */ /* 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 */ /* Update our position */
++(dls->last_backoff_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 */ /* Clamp it within min/max if we have them */
if (min_delay >= 0 && delay < min_delay) delay = min_delay; 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 */ /* Store it for next time */
dls->last_backoff_position = dls_schedule_position; 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) status_code; // XXXX no longer used.
(void) server; // XXXX no longer used. (void) server; // XXXX no longer used.
int increment = -1; int increment = -1;
int min_delay = 0, max_delay = INT_MAX; int min_delay = 0;
tor_assert(dls); 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 /* 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); min_delay = find_dl_min_delay(dls, get_options());
increment = download_status_schedule_get_delay(dls, increment = download_status_schedule_get_delay(dls, min_delay, now);
min_delay, max_delay, now);
} }
download_status_log_helper(item, !dls->increment_on, "failed", 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) time_t now)
{ {
int delay = -1; int delay = -1;
int min_delay = 0, max_delay = INT_MAX; int min_delay = 0;
tor_assert(dls); 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) if (dls->n_download_attempts < IMPOSSIBLE_TO_DOWNLOAD-1)
++dls->n_download_attempts; ++dls->n_download_attempts;
find_dl_min_and_max_delay(dls, get_options(), &min_delay, &max_delay); min_delay = find_dl_min_delay(dls, get_options());
delay = download_status_schedule_get_delay(dls, delay = download_status_schedule_get_delay(dls, min_delay, now);
min_delay, max_delay, now);
download_status_log_helper(item, dls->increment_on, "attempted", download_status_log_helper(item, dls->increment_on, "attempted",
"on failure", dls->n_download_attempts, "on failure", dls->n_download_attempts,

View File

@ -254,7 +254,7 @@ MOCK_DECL(STATIC int, directory_handle_command_post,(dir_connection_t *conn,
const char *body, const char *body,
size_t body_len)); size_t body_len));
STATIC int download_status_schedule_get_delay(download_status_t *dls, STATIC int download_status_schedule_get_delay(download_status_t *dls,
int min_delay, int max_delay, int min_delay,
time_t now); time_t now);
STATIC int handle_post_hs_descriptor(const char *url, const char *body); 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 compression_level_t choose_compression_level(ssize_t n_bytes);
STATIC const smartlist_t *find_dl_schedule(const download_status_t *dls, STATIC const smartlist_t *find_dl_schedule(const download_status_t *dls,
const or_options_t *options); const or_options_t *options);
STATIC void find_dl_min_and_max_delay(download_status_t *dls, STATIC int find_dl_min_delay(download_status_t *dls,
const or_options_t *options, const or_options_t *options);
int *min, int *max);
STATIC int next_random_exponential_delay(int delay, STATIC int next_random_exponential_delay(int delay,
int base_delay, int base_delay);
int max_delay);
STATIC void next_random_exponential_delay_range(int *low_bound_out, STATIC void next_random_exponential_delay_range(int *low_bound_out,
int *high_bound_out, int *high_bound_out,

View File

@ -3965,7 +3965,7 @@ test_dir_packages(void *arg)
} }
static void 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 = download_status_t dls_random =
{ 0, 0, 0, DL_SCHED_GENERIC, DL_WANT_AUTHORITY, { 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; int n_attempts = 0;
do { do {
increment = download_status_schedule_get_delay(&dls_random, increment = download_status_schedule_get_delay(&dls_random,
min_delay, max_delay, min_delay,
current_time); current_time);
log_debug(LD_DIR, "Min: %d, Max: %d, Inc: %d, Old Inc: %d", log_debug(LD_DIR, "Min: %d, Inc: %d, Old Inc: %d",
min_delay, max_delay, increment, old_increment); min_delay, increment, old_increment);
/* Regression test for 20534 and friends /* Regression test for 20534 and friends
* increment must always increase after the first */ * 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 */ /* Always increment the exponential backoff */
tt_int_op(increment, OP_GE, 1); tt_int_op(increment, OP_GE, 1);
} }
/* Test */ /* Test */
tt_int_op(increment, OP_GE, min_delay); tt_int_op(increment, OP_GE, min_delay);
tt_int_op(increment, OP_LE, max_delay);
/* Advance */ /* Advance */
if (dls_random.n_download_attempts < IMPOSSIBLE_TO_DOWNLOAD - 1) { 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 */ /* Try another maybe */
old_increment = increment; old_increment = increment;
} while (increment < max_delay && ++n_attempts < 1000); } while (++n_attempts < 1000);
done: done:
return; return;
@ -4015,19 +4014,13 @@ test_dir_download_status_random_backoff(void *arg)
(void)arg; (void)arg;
/* Do a standard test */ /* Do a standard test */
download_status_random_backoff_helper(0, 1000000); download_status_random_backoff_helper(0);
/* Regression test for 20534 and friends:
* try tighter bounds */
download_status_random_backoff_helper(0, 100);
/* regression tests for 17750: initial delay */ /* regression tests for 17750: initial delay */
download_status_random_backoff_helper(10, 1000); download_status_random_backoff_helper(10);
download_status_random_backoff_helper(20, 30); download_status_random_backoff_helper(20);
/* Pathological cases */ /* Pathological cases */
download_status_random_backoff_helper(0, 0); download_status_random_backoff_helper(INT_MAX/2);
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);
} }
static void static void