commit 6ff8c86ac663914901b4ea9ce64c20b59bec6011 Author: Matt Traudt sirmatt@ksu.edu Date: Wed Sep 13 16:35:15 2017 -0400
sched: change most asserts to non-fatal BUGs --- src/or/scheduler.c | 69 ++++++++++++++++++++++++++++++++++------------ src/or/scheduler_kist.c | 39 +++++++++++++++++++++----- src/or/scheduler_vanilla.c | 19 +++++++++++-- 3 files changed, 99 insertions(+), 28 deletions(-)
diff --git a/src/or/scheduler.c b/src/or/scheduler.c index cb93f93c6..8e4cec095 100644 --- a/src/or/scheduler.c +++ b/src/or/scheduler.c @@ -184,13 +184,19 @@ scheduler_evt_callback(evutil_socket_t fd, short events, void *arg)
log_debug(LD_SCHED, "Scheduler event callback called");
- tor_assert(run_sched_ev); - /* Run the scheduler. This is a mandatory function. */ + + /* We might as well assert on this. If this function doesn't exist, no cells + * are getting scheduled. Things are very broken. scheduler_t says the run() + * function is mandatory. */ tor_assert(the_scheduler->run); the_scheduler->run();
/* Schedule itself back in if it has more work. */ + + /* Again, might as well assert on this mandatory scheduler_t function. If it + * doesn't exist, there's no way to tell libevent to run the scheduler again + * in the future. */ tor_assert(the_scheduler->schedule); the_scheduler->schedule(); } @@ -224,14 +230,18 @@ scheduler_compare_channels, (const void *c1_v, const void *c2_v)) const circuitmux_policy_t *p1, *p2; uintptr_t p1_i, p2_i;
- tor_assert(c1_v); - tor_assert(c2_v); - c1 = (const channel_t *)(c1_v); c2 = (const channel_t *)(c2_v);
- tor_assert(c1); - tor_assert(c2); + IF_BUG_ONCE(!c1 || !c2) { + if (c1 && !c2) { + return -1; + } else if (c2 && !c1) { + return 1; + } else { + return -1; + } + }
if (c1 != c2) { if (circuitmux_get_policy(c1->cmux) == @@ -368,9 +378,12 @@ scheduler_free_all(void) MOCK_IMPL(void, scheduler_channel_doesnt_want_writes,(channel_t *chan)) { - tor_assert(chan); - - tor_assert(channels_pending); + IF_BUG_ONCE(!chan) { + return; + } + IF_BUG_ONCE(!channels_pending) { + return; + }
/* If it's already in pending, we can put it in waiting_to_write */ if (chan->scheduler_state == SCHED_CHAN_PENDING) { @@ -408,8 +421,12 @@ scheduler_channel_doesnt_want_writes,(channel_t *chan)) MOCK_IMPL(void, scheduler_channel_has_waiting_cells,(channel_t *chan)) { - tor_assert(chan); - tor_assert(channels_pending); + IF_BUG_ONCE(!chan) { + return; + } + IF_BUG_ONCE(!channels_pending) { + return; + }
/* First, check if this one also writeable */ if (chan->scheduler_state == SCHED_CHAN_WAITING_FOR_CELLS) { @@ -456,7 +473,13 @@ scheduler_init(void) { log_debug(LD_SCHED, "Initting scheduler");
- tor_assert(!run_sched_ev); + // Two '!' because we really do want to check if the pointer is non-NULL + IF_BUG_ONCE(!!run_sched_ev) { + log_warn(LD_SCHED, "We should not already have a libevent scheduler event." + "I'll clean the old one up, but this is odd."); + tor_event_free(run_sched_ev); + run_sched_ev = NULL; + } run_sched_ev = tor_event_new(tor_libevent_get_base(), -1, 0, scheduler_evt_callback, NULL); channels_pending = smartlist_new(); @@ -473,8 +496,12 @@ scheduler_init(void) MOCK_IMPL(void, scheduler_release_channel,(channel_t *chan)) { - tor_assert(chan); - tor_assert(channels_pending); + IF_BUG_ONCE(!chan) { + return; + } + IF_BUG_ONCE(!channels_pending) { + return; + }
if (chan->scheduler_state == SCHED_CHAN_PENDING) { if (smartlist_pos(channels_pending, chan) == -1) { @@ -500,8 +527,12 @@ scheduler_release_channel,(channel_t *chan)) void scheduler_channel_wants_writes(channel_t *chan) { - tor_assert(chan); - tor_assert(channels_pending); + IF_BUG_ONCE(!chan) { + return; + } + IF_BUG_ONCE(!channels_pending) { + return; + }
/* If it's already in waiting_to_write, we can put it in pending */ if (chan->scheduler_state == SCHED_CHAN_WAITING_TO_WRITE) { @@ -544,7 +575,9 @@ scheduler_channel_wants_writes(channel_t *chan) void scheduler_touch_channel(channel_t *chan) { - tor_assert(chan); + IF_BUG_ONCE(!chan) { + return; + }
if (chan->scheduler_state == SCHED_CHAN_PENDING) { /* Remove and re-add it */ diff --git a/src/or/scheduler_kist.c b/src/or/scheduler_kist.c index 0cedecd8f..aec8192ed 100644 --- a/src/or/scheduler_kist.c +++ b/src/or/scheduler_kist.c @@ -332,7 +332,9 @@ socket_can_write(socket_table_t *table, const channel_t *chan) { socket_table_ent_t *ent = NULL; ent = socket_table_search(table, chan); - tor_assert(ent); + IF_BUG_ONCE(!ent) { + return 1; // Just return true, saying that kist wouldn't limit the socket + }
int64_t kist_limit_space = (int64_t) (ent->limit - ent->written) / @@ -346,7 +348,9 @@ update_socket_info(socket_table_t *table, const channel_t *chan) { socket_table_ent_t *ent = NULL; ent = socket_table_search(table, chan); - tor_assert(ent); + IF_BUG_ONCE(!ent) { + return; // Whelp. Entry didn't exist for some reason so nothing to do. + } update_socket_info_impl(ent); }
@@ -356,7 +360,9 @@ update_socket_written(socket_table_t *table, channel_t *chan, size_t bytes) { socket_table_ent_t *ent = NULL; ent = socket_table_search(table, chan); - tor_assert(ent); + IF_BUG_ONCE(!ent) { + return; // Whelp. Entry didn't exist so nothing to do. + }
log_debug(LD_SCHED, "chan=%" PRIu64 " wrote %lu bytes, old was %" PRIi64, chan->global_identifier, bytes, ent->written); @@ -396,7 +402,9 @@ static int have_work(void) { smartlist_t *cp = get_channels_pending(); - tor_assert(cp); + IF_BUG_ONCE(!cp) { + return 0; // channels_pending doesn't exist so... no work? + } return smartlist_len(cp) > 0; }
@@ -441,7 +449,13 @@ static void kist_scheduler_init(void) { kist_scheduler_on_new_options(); - tor_assert(sched_run_interval > 0); + IF_BUG_ONCE(sched_run_interval <= 0) { + log_warn(LD_SCHED, "We are initing the KIST scheduler and noticed the " + "KISTSchedRunInterval is telling us to not use KIST. That's " + "weird! We'll continue using KIST, but at %dms.", + KIST_SCHED_RUN_INTERVAL_DEFAULT); + sched_run_interval = KIST_SCHED_RUN_INTERVAL_DEFAULT; + } }
/* Function of the scheduler interface: schedule() */ @@ -452,7 +466,13 @@ kist_scheduler_schedule(void) struct timeval next_run; int32_t diff; struct event *ev = get_run_sched_ev(); - tor_assert(ev); + IF_BUG_ONCE(!ev) { + log_warn(LD_SCHED, "Wow we don't have a scheduler event. That's really " + "weird! We can't really schedule a scheduling run with libevent " + "without it. So we're going to stop trying now and hope we have " + "one next time. If we never get one, we're broken."); + return; + } if (!have_work()) { return; } @@ -496,7 +516,12 @@ kist_scheduler_run(void) /* get best channel */ chan = smartlist_pqueue_pop(cp, scheduler_compare_channels, offsetof(channel_t, sched_heap_idx)); - tor_assert(chan); + IF_BUG_ONCE(!chan) { + /* Some-freaking-how a NULL got into the channels_pending. That should + * never happen, but it should be harmless to ignore it and keep looping. + */ + continue; + } outbuf_table_add(&outbuf_table, chan);
/* if we have switched to a new channel, consider writing the previous diff --git a/src/or/scheduler_vanilla.c b/src/or/scheduler_vanilla.c index d35b48c74..efc0e98de 100644 --- a/src/or/scheduler_vanilla.c +++ b/src/or/scheduler_vanilla.c @@ -29,7 +29,9 @@ static int have_work(void) { smartlist_t *cp = get_channels_pending(); - tor_assert(cp); + IF_BUG_ONCE(!cp) { + return 0; // channels_pending doesn't exist so... no work? + } return smartlist_len(cp) > 0; }
@@ -42,7 +44,13 @@ vanilla_scheduler_schedule(void) return; } struct event *ev = get_run_sched_ev(); - tor_assert(ev); + IF_BUG_ONCE(!ev) { + log_warn(LD_SCHED, "Wow we don't have a scheduler event. That's really " + "weird! We can't really schedule a scheduling run with libevent " + "without it. So we're going to stop trying now and hope we have " + "one next time. If we never get one, we're broken."); + return; + } event_active(ev, EV_TIMEOUT, 1); }
@@ -64,7 +72,12 @@ vanilla_scheduler_run(void) chan = smartlist_pqueue_pop(cp, scheduler_compare_channels, offsetof(channel_t, sched_heap_idx)); - tor_assert(chan); + IF_BUG_ONCE(!chan) { + /* Some-freaking-how a NULL got into the channels_pending. That should + * never happen, but it should be harmless to ignore it and keep looping. + */ + continue; + }
/* Figure out how many cells we can write */ n_cells = channel_num_cells_writeable(chan);