From 1e2ededb07844d2d1d361a27f9181013aac4e1fd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Thu, 29 Sep 2022 10:46:36 +0200 Subject: [PATCH 1/4] Add missing DbC check for name##_detach in ISC_REFCOUNT_IMPL macro The detach function in the ISC_REFCOUNT_IMPL macro was missing DbC checks, add them. --- lib/isc/include/isc/refcount.h | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/isc/include/isc/refcount.h b/lib/isc/include/isc/refcount.h index bf0a66e32b..e33cd7df25 100644 --- a/lib/isc/include/isc/refcount.h +++ b/lib/isc/include/isc/refcount.h @@ -174,6 +174,7 @@ isc_refcount_decrement(isc_refcount_t *target) { } \ \ void name##_detach(name##_t **ptrp) { \ + REQUIRE(ptrp != NULL && *ptrp != NULL); \ name##_t *ptr = *ptrp; \ *ptrp = NULL; \ name##_unref(ptr); \ From 09b50d2237192666d0fc72de3a29113f804924c9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Thu, 29 Sep 2022 18:06:05 +0200 Subject: [PATCH 2/4] Fix small problems in the isc_ratelimiter --- lib/isc/ratelimiter.c | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/lib/isc/ratelimiter.c b/lib/isc/ratelimiter.c index 8fe422ec73..f4ebcb22cf 100644 --- a/lib/isc/ratelimiter.c +++ b/lib/isc/ratelimiter.c @@ -57,10 +57,13 @@ ratelimiter_tick(void *arg); isc_result_t isc_ratelimiter_create(isc_loop_t *loop, isc_ratelimiter_t **ratelimiterp) { isc_ratelimiter_t *rl = NULL; - isc_mem_t *mctx = isc_loop_getmctx(loop); + isc_mem_t *mctx; + INSIST(loop != NULL); INSIST(ratelimiterp != NULL && *ratelimiterp == NULL); + mctx = isc_loop_getmctx(loop); + rl = isc_mem_get(mctx, sizeof(*rl)); *rl = (isc_ratelimiter_t){ .pertic = 1, @@ -237,14 +240,16 @@ isc_ratelimiter_shutdown(isc_ratelimiter_t *rl) { REQUIRE(VALID_RATELIMITER(rl)); LOCK(&rl->lock); - rl->state = isc_ratelimiter_shuttingdown; + if (rl->state != isc_ratelimiter_shuttingdown) { + rl->state = isc_ratelimiter_shuttingdown; - while ((event = ISC_LIST_HEAD(rl->pending)) != NULL) { - ISC_LIST_UNLINK(rl->pending, event, ev_ratelink); - event->ev_attributes |= ISC_EVENTATTR_CANCELED; - isc_task_send(event->ev_sender, &event); + while ((event = ISC_LIST_HEAD(rl->pending)) != NULL) { + ISC_LIST_UNLINK(rl->pending, event, ev_ratelink); + event->ev_attributes |= ISC_EVENTATTR_CANCELED; + isc_task_send(event->ev_sender, &event); + } + isc_loop_detach(&rl->loop); } - isc_loop_detach(&rl->loop); UNLOCK(&rl->lock); } From f7fc48a2bab0c4cae8fe9f9d9a26ff5dd3b63bc2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Thu, 29 Sep 2022 10:45:15 +0200 Subject: [PATCH 3/4] Add isc_ratelimiter API unit tests The isc_ratelimiter API was missing unit tests. Add a new set of unit tests for the isc_ratelimiter API. --- tests/isc/Makefile.am | 1 + tests/isc/ratelimiter_test.c | 334 +++++++++++++++++++++++++++++++++++ 2 files changed, 335 insertions(+) create mode 100644 tests/isc/ratelimiter_test.c diff --git a/tests/isc/Makefile.am b/tests/isc/Makefile.am index a9fd5a6137..e6a5d46ac0 100644 --- a/tests/isc/Makefile.am +++ b/tests/isc/Makefile.am @@ -32,6 +32,7 @@ check_PROGRAMS = \ quota_test \ radix_test \ random_test \ + ratelimiter_test\ regex_test \ result_test \ safe_test \ diff --git a/tests/isc/ratelimiter_test.c b/tests/isc/ratelimiter_test.c new file mode 100644 index 0000000000..7f82f3088a --- /dev/null +++ b/tests/isc/ratelimiter_test.c @@ -0,0 +1,334 @@ +/* + * Copyright (C) Internet Systems Consortium, Inc. ("ISC") + * + * SPDX-License-Identifier: MPL-2.0 + * + * This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, you can obtain one at https://mozilla.org/MPL/2.0/. + * + * See the COPYRIGHT file distributed with this work for additional + * information regarding copyright ownership. + */ + +#include +#include /* IWYU pragma: keep */ +#include +#include +#include +#include +#include +#include + +#define UNIT_TESTING +#include + +#include +#include +#include +#include +#include + +#include "ratelimiter.c" + +#include + +#define NS_PER_S 1000000000 /*%< Nanoseconds per second. */ + +isc_ratelimiter_t *rl = NULL; + +ISC_LOOP_TEST_IMPL(ratelimiter_create) { + rl = NULL; + expect_assert_failure(isc_ratelimiter_create(NULL, &rl)); + expect_assert_failure(isc_ratelimiter_create(mainloop, NULL)); + rl = (isc_ratelimiter_t *)&rl; + expect_assert_failure(isc_ratelimiter_create(mainloop, &rl)); + + rl = NULL; + isc_ratelimiter_create(mainloop, &rl); + isc_ratelimiter_shutdown(rl); + + isc_ratelimiter_detach(&rl); + + isc_loopmgr_shutdown(loopmgr); +} + +ISC_LOOP_TEST_IMPL(ratelimiter_shutdown) { + rl = NULL; + + expect_assert_failure(isc_ratelimiter_shutdown(NULL)); + expect_assert_failure(isc_ratelimiter_shutdown(rl)); + + isc_loopmgr_shutdown(loopmgr); +} + +ISC_LOOP_TEST_IMPL(ratelimiter_detach) { + rl = NULL; + + expect_assert_failure(isc_ratelimiter_detach(NULL)); + expect_assert_failure(isc_ratelimiter_detach(&rl)); + + isc_loopmgr_shutdown(loopmgr); +} + +static int ticks = 0; +static isc_task_t *rl_task = NULL; +static isc_time_t start_time; +static isc_time_t tick_time; + +static void +tick(isc_task_t *task, isc_event_t *event) { + assert_ptr_equal(task, rl_task); + isc_event_free(&event); + + ticks++; + + assert_int_equal(isc_time_now(&tick_time), ISC_R_SUCCESS); + + isc_loopmgr_shutdown(loopmgr); + + isc_task_detach(&rl_task); + isc_ratelimiter_shutdown(rl); + isc_ratelimiter_detach(&rl); +} + +ISC_LOOP_SETUP_IMPL(ratelimiter_common) { + isc_result_t result = isc_task_create(taskmgr, &rl_task, 0); + assert_int_equal(result, ISC_R_SUCCESS); + + rl = NULL; + isc_time_set(&tick_time, 0, 0); + assert_int_equal(isc_time_now(&start_time), ISC_R_SUCCESS); + isc_ratelimiter_create(mainloop, &rl); +} + +ISC_LOOP_SETUP_IMPL(ratelimiter_enqueue) { + ticks = 0; + setup_loop_ratelimiter_common(arg); +} + +ISC_LOOP_TEARDOWN_IMPL(ratelimiter_enqueue) { assert_int_equal(ticks, 1); } + +ISC_LOOP_TEST_SETUP_TEARDOWN_IMPL(ratelimiter_enqueue) { + isc_result_t result; + isc_event_t *event = NULL; + + event = isc_event_allocate(mctx, NULL, ISC_TASKEVENT_TEST, tick, NULL, + sizeof(isc_event_t)); + assert_non_null(event); + + result = isc_ratelimiter_enqueue(rl, rl_task, &event); + assert_int_equal(result, ISC_R_SUCCESS); +} + +ISC_LOOP_SETUP_IMPL(ratelimiter_enqueue_shutdown) { + ticks = 0; + setup_loop_ratelimiter_common(arg); +} + +ISC_LOOP_TEARDOWN_IMPL(ratelimiter_enqueue_shutdown) { + assert_int_equal(ticks, 1); +} + +ISC_LOOP_TEST_SETUP_TEARDOWN_IMPL(ratelimiter_enqueue_shutdown) { + isc_event_t *event = NULL; + + event = isc_event_allocate(mctx, NULL, ISC_TASKEVENT_TEST, tick, NULL, + sizeof(isc_event_t)); + assert_non_null(event); + + expect_assert_failure(isc_ratelimiter_enqueue(NULL, rl_task, &event)); + expect_assert_failure(isc_ratelimiter_enqueue(rl, NULL, &event)); + expect_assert_failure(isc_ratelimiter_enqueue(rl, rl_task, NULL)); + expect_assert_failure( + isc_ratelimiter_enqueue(rl, rl_task, &(isc_event_t *){ NULL })); + + assert_int_equal(isc_ratelimiter_enqueue(rl, rl_task, &event), + ISC_R_SUCCESS); + + isc_ratelimiter_shutdown(rl); + + event = isc_event_allocate(mctx, NULL, ISC_TASKEVENT_TEST, tick, NULL, + sizeof(isc_event_t)); + assert_non_null(event); + + assert_int_equal(isc_ratelimiter_enqueue(rl, rl_task, &event), + ISC_R_SHUTTINGDOWN); + + isc_event_free(&event); +} + +ISC_LOOP_SETUP_IMPL(ratelimiter_dequeue) { + ticks = 0; + setup_loop_ratelimiter_common(arg); +} + +ISC_LOOP_TEARDOWN_IMPL(ratelimiter_dequeue) { /* */ + assert_int_equal(ticks, 1); +} + +ISC_LOOP_TEST_SETUP_TEARDOWN_IMPL(ratelimiter_dequeue) { + isc_event_t *event = NULL; + + event = isc_event_allocate(mctx, NULL, ISC_TASKEVENT_TEST, tick, NULL, + sizeof(isc_event_t)); + assert_non_null(event); + assert_int_equal( + isc_ratelimiter_enqueue(rl, rl_task, &(isc_event_t *){ event }), + ISC_R_SUCCESS); + assert_int_equal(isc_ratelimiter_dequeue(rl, event), ISC_R_SUCCESS); + isc_event_free(&event); + assert_null(event); + + /* This event didn't get scheduled */ + event = isc_event_allocate(mctx, NULL, ISC_TASKEVENT_TEST, tick, NULL, + sizeof(isc_event_t)); + assert_non_null(event); + assert_int_equal(isc_ratelimiter_dequeue(rl, event), ISC_R_NOTFOUND); + assert_int_equal(isc_ratelimiter_enqueue(rl, rl_task, &event), + ISC_R_SUCCESS); + assert_null(event); +} + +static isc_time_t tock_time; + +static void +tock(isc_task_t *task, isc_event_t *event) { + assert_ptr_equal(task, rl_task); + isc_event_free(&event); + + ticks++; + assert_int_equal(isc_time_now(&tock_time), ISC_R_SUCCESS); +} + +ISC_LOOP_SETUP_IMPL(ratelimiter_pertick_interval) { + ticks = 0; + isc_time_set(&tock_time, 0, 0); + setup_loop_ratelimiter_common(arg); +} + +ISC_LOOP_TEARDOWN_IMPL(ratelimiter_pertick_interval) { + uint64_t t = isc_time_microdiff(&tick_time, &tock_time); + assert_int_equal(ticks, 2); + assert_true(t >= 1000000); + + t = isc_time_microdiff(&tock_time, &start_time); + assert_true(t < 1000000); +} + +ISC_LOOP_TEST_SETUP_TEARDOWN_IMPL(ratelimiter_pertick_interval) { + isc_event_t *event = NULL; + isc_interval_t interval; + + isc_interval_set(&interval, 1, NS_PER_S / 10); + + expect_assert_failure(isc_ratelimiter_setinterval(NULL, &interval)); + expect_assert_failure(isc_ratelimiter_setinterval(rl, NULL)); + expect_assert_failure(isc_ratelimiter_setpertic(NULL, 1)); + expect_assert_failure(isc_ratelimiter_setpertic(rl, 0)); + expect_assert_failure(isc_ratelimiter_setpushpop(NULL, false)); + + isc_ratelimiter_setinterval(rl, &interval); + isc_ratelimiter_setpertic(rl, 1); + isc_ratelimiter_setpushpop(rl, false); + + event = isc_event_allocate(mctx, NULL, ISC_TASKEVENT_TEST, tock, NULL, + sizeof(isc_event_t)); + assert_non_null(event); + + assert_int_equal(isc_ratelimiter_enqueue(rl, rl_task, &event), + ISC_R_SUCCESS); + + event = isc_event_allocate(mctx, NULL, ISC_TASKEVENT_TEST, tick, NULL, + sizeof(isc_event_t)); + assert_non_null(event); + + assert_int_equal(isc_ratelimiter_enqueue(rl, rl_task, &event), + ISC_R_SUCCESS); +} + +ISC_LOOP_SETUP_IMPL(ratelimiter_pushpop) { + ticks = 0; + isc_time_set(&tock_time, 0, 0); + setup_loop_ratelimiter_common(arg); +} + +ISC_LOOP_TEARDOWN_IMPL(ratelimiter_pushpop) { + uint64_t t = isc_time_microdiff(&tock_time, &tick_time); + assert_int_equal(ticks, 2); + assert_true(t < 1000000); +} + +ISC_LOOP_TEST_SETUP_TEARDOWN_IMPL(ratelimiter_pushpop) { + isc_event_t *event = NULL; + isc_interval_t interval; + + isc_interval_set(&interval, 1, NS_PER_S / 10); + + isc_ratelimiter_setinterval(rl, &interval); + isc_ratelimiter_setpertic(rl, 2); + isc_ratelimiter_setpushpop(rl, true); + + event = isc_event_allocate(mctx, NULL, ISC_TASKEVENT_TEST, tick, NULL, + sizeof(isc_event_t)); + assert_non_null(event); + + assert_int_equal( + isc_ratelimiter_enqueue(rl, rl_task, &(isc_event_t *){ event }), + ISC_R_SUCCESS); + + event = isc_event_allocate(mctx, NULL, ISC_TASKEVENT_TEST, tock, NULL, + sizeof(isc_event_t)); + assert_non_null(event); + + assert_int_equal( + isc_ratelimiter_enqueue(rl, rl_task, &(isc_event_t *){ event }), + ISC_R_SUCCESS); +} + +static int +setup_test(void **state) { + int r; + + r = setup_loopmgr(state); + if (r != 0) { + return (r); + } + r = setup_taskmgr(state); + if (r != 0) { + return (r); + } + + return (0); +} + +static int +teardown_test(void **state) { + int r; + + r = teardown_taskmgr(state); + if (r != 0) { + return (r); + } + r = teardown_loopmgr(state); + if (r != 0) { + return (r); + } + + return (0); +} + +ISC_TEST_LIST_START + +ISC_TEST_ENTRY_CUSTOM(ratelimiter_create, setup_test, teardown_test) +ISC_TEST_ENTRY_CUSTOM(ratelimiter_shutdown, setup_test, teardown_test) +ISC_TEST_ENTRY_CUSTOM(ratelimiter_detach, setup_test, teardown_test) +ISC_TEST_ENTRY_CUSTOM(ratelimiter_enqueue, setup_test, teardown_test) +ISC_TEST_ENTRY_CUSTOM(ratelimiter_enqueue_shutdown, setup_test, teardown_test) +ISC_TEST_ENTRY_CUSTOM(ratelimiter_dequeue, setup_test, teardown_test) +ISC_TEST_ENTRY_CUSTOM(ratelimiter_pertick_interval, setup_test, teardown_test) +ISC_TEST_ENTRY_CUSTOM(ratelimiter_pushpop, setup_test, teardown_test) + +ISC_TEST_LIST_END + +ISC_TEST_MAIN From 477eb22c1274164e0369d21769e5f7ec04745997 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Thu, 29 Sep 2022 09:30:20 +0200 Subject: [PATCH 4/4] Refactor isc_ratelimiter API Because the dns_zonemgr_create() was run before the loopmgr was started, the isc_ratelimiter API was more complicated that it had to be. Move the dns_zonemgr_create() to run_server() task which is run on the main loop, and simplify the isc_ratelimiter API implementation. The isc_timer is now created in the isc_ratelimiter_create() and starting the timer is now separate async task as is destroying the timer in case it's not launched from the loop it was created on. The ratelimiter tick now doesn't have to create and destroy timer logic and just stops the timer when there's no more work to do. This should also solve all the races that were causing the isc_ratelimiter to be left dangling because the timer was stopped before the last reference would be detached. --- bin/named/server.c | 10 +-- lib/dns/zone.c | 42 ++------- lib/isc/include/isc/ratelimiter.h | 4 +- lib/isc/ratelimiter.c | 140 ++++++++++++++++++++---------- 4 files changed, 110 insertions(+), 86 deletions(-) diff --git a/bin/named/server.c b/bin/named/server.c index f197135b71..26ad34a1e2 100644 --- a/bin/named/server.c +++ b/bin/named/server.c @@ -10008,6 +10008,11 @@ run_server(isc_task_t *task, isc_event_t *event) { isc_event_free(&event); + CHECKFATAL(dns_zonemgr_create(named_g_mctx, named_g_loopmgr, + named_g_taskmgr, named_g_netmgr, + &server->zonemgr), + "dns_zonemgr_create"); + CHECKFATAL(dns_dispatchmgr_create(named_g_mctx, named_g_netmgr, &named_g_dispatchmgr), "creating dispatch manager"); @@ -10322,11 +10327,6 @@ named_server_create(isc_mem_t *mctx, named_server_t **serverp) { server->sighup = isc_signal_new( named_g_loopmgr, named_server_reloadwanted, server, SIGHUP); - CHECKFATAL(dns_zonemgr_create(named_g_mctx, named_g_loopmgr, - named_g_taskmgr, named_g_netmgr, - &server->zonemgr), - "dns_zonemgr_create"); - CHECKFATAL(isc_stats_create(server->mctx, &server->sockstats, isc_sockstatscounter_max), "isc_stats_create"); diff --git a/lib/dns/zone.c b/lib/dns/zone.c index 9a7ab8f89b..08e0000bd8 100644 --- a/lib/dns/zone.c +++ b/lib/dns/zone.c @@ -18865,7 +18865,7 @@ dns_zonemgr_create(isc_mem_t *mctx, isc_loopmgr_t *loopmgr, dns_zonemgr_t **zmgrp) { dns_zonemgr_t *zmgr; isc_result_t result; - isc_loop_t *mainloop = isc_loop_main(loopmgr); + isc_loop_t *loop = isc_loop_current(loopmgr); REQUIRE(mctx != NULL); REQUIRE(loopmgr != NULL); @@ -18899,35 +18899,11 @@ dns_zonemgr_create(isc_mem_t *mctx, isc_loopmgr_t *loopmgr, /* Unreachable lock. */ isc_rwlock_init(&zmgr->urlock, 0, 0); - result = isc_ratelimiter_create(mainloop, &zmgr->checkdsrl); - INSIST(result == ISC_R_SUCCESS); - if (result != ISC_R_SUCCESS) { - goto free_urlock; - } - - result = isc_ratelimiter_create(mainloop, &zmgr->notifyrl); - INSIST(result == ISC_R_SUCCESS); - if (result != ISC_R_SUCCESS) { - goto free_checkdsrl; - } - - result = isc_ratelimiter_create(mainloop, &zmgr->refreshrl); - INSIST(result == ISC_R_SUCCESS); - if (result != ISC_R_SUCCESS) { - goto free_notifyrl; - } - - result = isc_ratelimiter_create(mainloop, &zmgr->startupnotifyrl); - INSIST(result == ISC_R_SUCCESS); - if (result != ISC_R_SUCCESS) { - goto free_refreshrl; - } - - result = isc_ratelimiter_create(mainloop, &zmgr->startuprefreshrl); - INSIST(result == ISC_R_SUCCESS); - if (result != ISC_R_SUCCESS) { - goto free_startupnotifyrl; - } + isc_ratelimiter_create(loop, &zmgr->checkdsrl); + isc_ratelimiter_create(loop, &zmgr->notifyrl); + isc_ratelimiter_create(loop, &zmgr->refreshrl); + isc_ratelimiter_create(loop, &zmgr->startupnotifyrl); + isc_ratelimiter_create(loop, &zmgr->startuprefreshrl); zmgr->zonetasks = isc_mem_get( zmgr->mctx, zmgr->workers * sizeof(zmgr->zonetasks[0])); @@ -19012,19 +18988,15 @@ free_zonetasks: isc_ratelimiter_shutdown(zmgr->startuprefreshrl); isc_ratelimiter_detach(&zmgr->startuprefreshrl); -free_startupnotifyrl: isc_ratelimiter_shutdown(zmgr->startupnotifyrl); isc_ratelimiter_detach(&zmgr->startupnotifyrl); -free_refreshrl: isc_ratelimiter_shutdown(zmgr->refreshrl); isc_ratelimiter_detach(&zmgr->refreshrl); -free_notifyrl: isc_ratelimiter_shutdown(zmgr->notifyrl); isc_ratelimiter_detach(&zmgr->notifyrl); -free_checkdsrl: isc_ratelimiter_shutdown(zmgr->checkdsrl); isc_ratelimiter_detach(&zmgr->checkdsrl); -free_urlock: + isc_rwlock_destroy(&zmgr->urlock); isc_rwlock_destroy(&zmgr->rwlock); isc_mem_put(zmgr->mctx, zmgr, sizeof(*zmgr)); diff --git a/lib/isc/include/isc/ratelimiter.h b/lib/isc/include/isc/ratelimiter.h index 5eb54f341f..1f4b8d2d7e 100644 --- a/lib/isc/include/isc/ratelimiter.h +++ b/lib/isc/include/isc/ratelimiter.h @@ -41,8 +41,8 @@ ISC_LANG_BEGINDECLS ***** Functions. *****/ -isc_result_t -isc_ratelimiter_create(isc_loop_t *loop, isc_ratelimiter_t **ratelimiterp); +void +isc_ratelimiter_create(isc_loop_t *loop, isc_ratelimiter_t **rlp); /*%< * Create a rate limiter. The execution interval is initially undefined. */ diff --git a/lib/isc/ratelimiter.c b/lib/isc/ratelimiter.c index f4ebcb22cf..07bc45c236 100644 --- a/lib/isc/ratelimiter.c +++ b/lib/isc/ratelimiter.c @@ -52,15 +52,21 @@ struct isc_ratelimiter { }; static void -ratelimiter_tick(void *arg); +isc__ratelimiter_tick(void *arg); -isc_result_t -isc_ratelimiter_create(isc_loop_t *loop, isc_ratelimiter_t **ratelimiterp) { +static void +isc__ratelimiter_start(void *arg); + +static void +isc__ratelimiter_doshutdown(void *arg); + +void +isc_ratelimiter_create(isc_loop_t *loop, isc_ratelimiter_t **rlp) { isc_ratelimiter_t *rl = NULL; isc_mem_t *mctx; - INSIST(loop != NULL); - INSIST(ratelimiterp != NULL && *ratelimiterp == NULL); + REQUIRE(loop != NULL); + REQUIRE(rlp != NULL && *rlp == NULL); mctx = isc_loop_getmctx(loop); @@ -77,10 +83,11 @@ isc_ratelimiter_create(isc_loop_t *loop, isc_ratelimiter_t **ratelimiterp) { isc_interval_set(&rl->interval, 0, 0); ISC_LIST_INIT(rl->pending); + isc_timer_create(rl->loop, isc__ratelimiter_tick, rl, &rl->timer); + isc_mutex_init(&rl->lock); - *ratelimiterp = rl; - return (ISC_R_SUCCESS); + *rlp = rl; } void @@ -90,10 +97,7 @@ isc_ratelimiter_setinterval(isc_ratelimiter_t *rl, isc_interval_t *interval) { LOCK(&rl->lock); rl->interval = *interval; - /* - * If the timer is currently running, its rate will change during - * the next tick. - */ + /* The interval will be adjusted on the next tick */ UNLOCK(&rl->lock); } @@ -116,6 +120,37 @@ isc_ratelimiter_setpushpop(isc_ratelimiter_t *rl, bool pushpop) { UNLOCK(&rl->lock); } +static void +isc__ratelimiter_start(void *arg) { + isc_ratelimiter_t *rl = arg; + isc_interval_t interval; + + REQUIRE(VALID_RATELIMITER(rl)); + + LOCK(&rl->lock); + switch (rl->state) { + case isc_ratelimiter_ratelimited: + /* The first tick happens immediately */ + isc_interval_set(&interval, 0, 0); + isc_timer_start(rl->timer, isc_timertype_once, &interval); + break; + case isc_ratelimiter_shuttingdown: + /* The ratelimiter is shutting down */ + break; + case isc_ratelimiter_idle: + /* + * This could happen if we are changing the interval on the + * ratelimiter, but all the events were processed and the timer + * was stopped before the new interval could be applied. + */ + break; + default: + UNREACHABLE(); + } + UNLOCK(&rl->lock); + isc_ratelimiter_detach(&rl); +} + isc_result_t isc_ratelimiter_enqueue(isc_ratelimiter_t *rl, isc_task_t *task, isc_event_t **eventp) { @@ -136,9 +171,9 @@ isc_ratelimiter_enqueue(isc_ratelimiter_t *rl, isc_task_t *task, case isc_ratelimiter_idle: /* Start the ratelimiter */ isc_ratelimiter_ref(rl); - isc_async_run(rl->loop, ratelimiter_tick, rl); + isc_async_run(rl->loop, isc__ratelimiter_start, rl); rl->state = isc_ratelimiter_ratelimited; - /* FALLTHROUGH */ + FALLTHROUGH; case isc_ratelimiter_ratelimited: event->ev_sender = task; *eventp = NULL; @@ -175,11 +210,10 @@ isc_ratelimiter_dequeue(isc_ratelimiter_t *rl, isc_event_t *event) { } static void -ratelimiter_tick(void *arg) { +isc__ratelimiter_tick(void *arg) { isc_ratelimiter_t *rl = (isc_ratelimiter_t *)arg; isc_event_t *event; uint32_t pertic; - bool do_destroy = false; ISC_LIST(isc_event_t) pending; REQUIRE(VALID_RATELIMITER(rl)); @@ -187,70 +221,88 @@ ratelimiter_tick(void *arg) { ISC_LIST_INIT(pending); LOCK(&rl->lock); + + REQUIRE(rl->timer != NULL); + if (rl->state == isc_ratelimiter_shuttingdown) { - UNLOCK(&rl->lock); - do_destroy = (rl->timer != NULL); - goto done; + INSIST(EMPTY(rl->pending)); + goto unlock; } - if (rl->timer == NULL) { - isc_timer_create(rl->loop, ratelimiter_tick, rl, &rl->timer); - } - - /* - * If the timer was already running with a different rate, - * this updates it to the correct one. - */ - isc_timer_start(rl->timer, isc_timertype_ticker, &rl->interval); - pertic = rl->pertic; while (pertic != 0) { - pertic--; event = ISC_LIST_HEAD(rl->pending); if (event != NULL) { /* There is work to do. Let's do it after unlocking. */ ISC_LIST_UNLINK(rl->pending, event, ev_ratelink); ISC_LIST_APPEND(pending, event, ev_ratelink); } else { - /* There's no more work to do, destroy the timer */ - do_destroy = true; + /* + * We processed all the scheduled work, but there's a + * room for at least one more event (we haven't consumed + * all of the "pertick"), so we can stop the ratelimiter + * now, and don't worry about isc_ratelimiter_enqueue() + * sending an extra event immediately. + */ rl->state = isc_ratelimiter_idle; break; } + pertic--; } + + if (rl->state != isc_ratelimiter_idle) { + /* Reschedule the timer */ + isc_timer_start(rl->timer, isc_timertype_once, &rl->interval); + } +unlock: UNLOCK(&rl->lock); while ((event = ISC_LIST_HEAD(pending)) != NULL) { ISC_LIST_UNLINK(pending, event, ev_ratelink); isc_task_send(event->ev_sender, &event); } +} -done: - /* No work left to do. Stop and destroy the timer. */ - if (do_destroy) { - isc_timer_destroy(&rl->timer); - isc_ratelimiter_detach(&rl); - } +void +isc__ratelimiter_doshutdown(void *arg) { + isc_ratelimiter_t *rl = arg; + + REQUIRE(VALID_RATELIMITER(rl)); + + LOCK(&rl->lock); + INSIST(rl->state == isc_ratelimiter_shuttingdown); + INSIST(EMPTY(rl->pending)); + + isc_timer_stop(rl->timer); + isc_timer_destroy(&rl->timer); + isc_loop_detach(&rl->loop); + UNLOCK(&rl->lock); + isc_ratelimiter_detach(&rl); } void isc_ratelimiter_shutdown(isc_ratelimiter_t *rl) { isc_event_t *event; + ISC_LIST(isc_event_t) pending; REQUIRE(VALID_RATELIMITER(rl)); + ISC_LIST_INIT(pending); + LOCK(&rl->lock); if (rl->state != isc_ratelimiter_shuttingdown) { rl->state = isc_ratelimiter_shuttingdown; - - while ((event = ISC_LIST_HEAD(rl->pending)) != NULL) { - ISC_LIST_UNLINK(rl->pending, event, ev_ratelink); - event->ev_attributes |= ISC_EVENTATTR_CANCELED; - isc_task_send(event->ev_sender, &event); - } - isc_loop_detach(&rl->loop); + ISC_LIST_MOVE(pending, rl->pending); + isc_ratelimiter_ref(rl); + isc_async_run(rl->loop, isc__ratelimiter_doshutdown, rl); } UNLOCK(&rl->lock); + + while ((event = ISC_LIST_HEAD(pending)) != NULL) { + ISC_LIST_UNLINK(pending, event, ev_ratelink); + event->ev_attributes |= ISC_EVENTATTR_CANCELED; + isc_task_send(event->ev_sender, &event); + } } static void