Merge branch '3252-stop-timer-before-detach' into 'main'

Don't use reference counting in isc_timer unit

Closes #3252

See merge request isc-projects/bind9!6067

bin/named/server.c

@@ -9893,10 +9893,10 @@ shutdown_server(isc_task_t *task, isc_event_t *event) {
 		isc_mem_put(server->mctx, nsc, sizeof(*nsc));
 	}
 
-	isc_timer_detach(&server->interface_timer);
-	isc_timer_detach(&server->heartbeat_timer);
-	isc_timer_detach(&server->pps_timer);
-	isc_timer_detach(&server->tat_timer);
+	isc_timer_destroy(&server->interface_timer);
+	isc_timer_destroy(&server->heartbeat_timer);
+	isc_timer_destroy(&server->pps_timer);
+	isc_timer_destroy(&server->tat_timer);
 
 	ns_interfacemgr_detach(&server->interfacemgr);
 

lib/dns/catz.c

@@ -765,7 +765,7 @@ dns_catz_zone_detach(dns_catz_zone_t **zonep) {
 			isc_ht_destroy(&zone->entries);
 		}
 		zone->magic = 0;
-		isc_timer_detach(&zone->updatetimer);
+		isc_timer_destroy(&zone->updatetimer);
 		if (zone->db_registered) {
 			INSIST(dns_db_updatenotify_unregister(
 				       zone->db, dns_catz_dbupdate_callback,

lib/dns/nta.c

@@ -76,7 +76,7 @@ nta_detach(isc_mem_t *mctx, dns_nta_t **ntap) {
 		if (nta->timer != NULL) {
 			(void)isc_timer_reset(
 				nta->timer, isc_timertype_inactive, NULL, true);
-			isc_timer_detach(&nta->timer);
+			isc_timer_destroy(&nta->timer);
 		}
 		if (dns_rdataset_isassociated(&nta->rdataset)) {
 			dns_rdataset_disassociate(&nta->rdataset);
@@ -294,7 +294,7 @@ settimer(dns_ntatable_t *ntatable, dns_nta_t *nta, uint32_t lifetime) {
 	result = isc_timer_reset(nta->timer, isc_timertype_ticker, &interval,
 				 false);
 	if (result != ISC_R_SUCCESS) {
-		isc_timer_detach(&nta->timer);
+		isc_timer_destroy(&nta->timer);
 	}
 	return (result);
 }
@@ -481,7 +481,7 @@ again:
 		if (nta->timer != NULL) {
 			(void)isc_timer_reset(
 				nta->timer, isc_timertype_inactive, NULL, true);
-			isc_timer_detach(&nta->timer);
+			isc_timer_destroy(&nta->timer);
 		}
 
 		result = deletenode(ntatable, foundname);

lib/dns/resolver.c

@@ -4379,7 +4379,7 @@ fctx_destroy(fetchctx_t *fctx, bool exiting) {
 	dns_db_detach(&fctx->cache);
 	dns_adb_detach(&fctx->adb);
 
-	isc_timer_detach(&fctx->timer);
+	isc_timer_destroy(&fctx->timer);
 
 	dns_resolver_detach(&fctx->res);
 
@@ -4927,7 +4927,7 @@ cleanup_mctx:
 	dns_db_detach(&fctx->cache);
 
 cleanup_timer:
-	isc_timer_detach(&fctx->timer);
+	isc_timer_destroy(&fctx->timer);
 
 cleanup_qmessage:
 	dns_message_detach(&fctx->qmessage);
@@ -10118,7 +10118,7 @@ destroy(dns_resolver_t *res) {
 	dns_resolver_reset_ds_digests(res);
 	dns_badcache_destroy(&res->badcache);
 	dns_resolver_resetmustbesecure(res);
-	isc_timer_detach(&res->spillattimer);
+	isc_timer_destroy(&res->spillattimer);
 	res->magic = 0;
 	isc_mem_putanddetach(&res->mctx, res, sizeof(*res));
 }

lib/dns/rpz.c

@@ -2189,7 +2189,7 @@ rpz_detach(dns_rpz_zone_t **rpzp) {
 
 		isc_timer_reset(rpz->updatetimer, isc_timertype_inactive, NULL,
 				true);
-		isc_timer_detach(&rpz->updatetimer);
+		isc_timer_destroy(&rpz->updatetimer);
 
 		isc_ht_destroy(&rpz->nodes);
 

lib/dns/zone.c

@@ -15088,8 +15088,7 @@ zone_shutdown(isc_task_t *task, isc_event_t *event) {
 	forward_cancel(zone);
 
 	if (zone->timer != NULL) {
-		zone_timer_stop(zone);
-		isc_timer_detach(&zone->timer);
+		isc_timer_destroy(&zone->timer);
 		isc_refcount_decrement(&zone->irefs);
 	}
 

lib/isc/include/isc/timer.h

@@ -182,25 +182,9 @@ isc_timer_reset(isc_timer_t *timer, isc_timertype_t type,
  */
 
 void
-isc_timer_attach(isc_timer_t *timer, isc_timer_t **timerp);
+isc_timer_destroy(isc_timer_t **timerp);
 /*%<
- * Attach *timerp to timer.
- *
- * Requires:
- *
- *\li	'timer' is a valid timer.
- *
- *\li	'timerp' points to a NULL timer.
- *
- * Ensures:
- *
- *\li	*timerp is attached to timer.
- */
-
-void
-isc_timer_detach(isc_timer_t **timerp);
-/*%<
- * Detach *timerp from its timer.
+ * Destroy *timerp.
  *
  * Requires:
  *
@@ -210,9 +194,6 @@ isc_timer_detach(isc_timer_t **timerp);
  *
  *\li	*timerp is NULL.
  *
- *\li	If '*timerp' is the last reference to the timer,
- *	then:
- *
  *\code
  *		The timer will be shutdown
  *
@@ -221,9 +202,13 @@ isc_timer_detach(isc_timer_t **timerp);
  *		All resources used by the timer have been freed
  *
  *		Any events already posted by the timer will be purged.
- *		Therefore, if isc_timer_detach() is called in the context
+ *		Therefore, if isc_timer_destroy() is called in the context
  *		of the timer's task, it is guaranteed that no more
  *		timer event callbacks will run after the call.
+ *
+ *		If this function is called from the timer event callback
+ *		the event itself must be destroyed before the timer
+ *		itself.
  *\endcode
  */
 

lib/isc/ratelimiter.c

@@ -229,6 +229,7 @@ void
 isc_ratelimiter_shutdown(isc_ratelimiter_t *rl) {
 	isc_event_t *ev;
 	isc_task_t *task;
+	isc_result_t result;
 
 	REQUIRE(rl != NULL);
 
@@ -243,7 +244,11 @@ isc_ratelimiter_shutdown(isc_ratelimiter_t *rl) {
 	}
 	task = NULL;
 	isc_task_attach(rl->task, &task);
-	isc_timer_detach(&rl->timer);
+
+	result = isc_timer_reset(rl->timer, isc_timertype_inactive, NULL,
+				 false);
+	RUNTIME_CHECK(result == ISC_R_SUCCESS);
+	isc_timer_destroy(&rl->timer);
 
 	/*
 	 * Send an event to our task.  The delivery of this event

lib/isc/tests/task_test.c

@@ -304,8 +304,8 @@ basic(void **state) {
 	isc_task_detach(&task4);
 
 	sleep(10);
-	isc_timer_detach(&ti1);
-	isc_timer_detach(&ti2);
+	isc_timer_destroy(&ti1);
+	isc_timer_destroy(&ti2);
 }
 
 /*

lib/isc/tests/timer_test.c

@@ -241,14 +241,14 @@ ticktock(isc_task_t *task, isc_event_t *event) {
 	isc_mutex_unlock(&lasttime_mx);
 	subthread_assert_result_equal(result, ISC_R_SUCCESS);
 
+	isc_event_free(&event);
+
 	if (atomic_load(&eventcnt) == nevents) {
 		result = isc_time_now(&endtime);
 		subthread_assert_result_equal(result, ISC_R_SUCCESS);
-		isc_timer_detach(&timer);
+		isc_timer_destroy(&timer);
 		isc_task_shutdown(task);
 	}
-
-	isc_event_free(&event);
 }
 
 /*
@@ -312,9 +312,10 @@ test_idle(isc_task_t *task, isc_event_t *event) {
 
 	subthread_assert_int_equal(event->ev_type, ISC_TIMEREVENT_ONCE);
 
-	isc_timer_detach(&timer);
-	isc_task_shutdown(task);
 	isc_event_free(&event);
+
+	isc_timer_destroy(&timer);
+	isc_task_shutdown(task);
 }
 
 /* timer type once idles out */
@@ -387,14 +388,15 @@ test_reset(isc_task_t *task, isc_event_t *event) {
 						 &interval, false);
 			subthread_assert_result_equal(result, ISC_R_SUCCESS);
 		}
+
+		isc_event_free(&event);
 	} else {
 		subthread_assert_int_equal(event->ev_type, ISC_TIMEREVENT_ONCE);
 
-		isc_timer_detach(&timer);
+		isc_event_free(&event);
+		isc_timer_destroy(&timer);
 		isc_task_shutdown(task);
 	}
-
-	isc_event_free(&event);
 }
 
 static void
@@ -545,8 +547,8 @@ purge(void **state) {
 
 	assert_int_equal(atomic_load(&eventcnt), 1);
 
-	isc_timer_detach(&tickertimer);
-	isc_timer_detach(&oncetimer);
+	isc_timer_destroy(&tickertimer);
+	isc_timer_destroy(&oncetimer);
 	isc_task_destroy(&task1);
 	isc_task_destroy(&task2);
 }

lib/isc/timer.c

@@ -59,9 +59,9 @@ struct isc_timer {
 	unsigned int magic;
 	isc_timermgr_t *manager;
 	isc_mutex_t lock;
-	isc_refcount_t references;
 	/*! Locked by timer lock. */
 	isc_time_t idle;
+	ISC_LIST(isc_timerevent_t) active;
 	/*! Locked by manager lock. */
 	isc_timertype_t type;
 	isc_interval_t interval;
@@ -70,7 +70,6 @@ struct isc_timer {
 	void *arg;
 	unsigned int index;
 	isc_time_t due;
-	ISC_LIST(isc_timerevent_t) active;
 	LINK(isc_timer_t) link;
 };
 
@@ -201,13 +200,14 @@ timerevent_destroy(isc_event_t *event0) {
 	isc_timer_t *timer = event0->ev_destroy_arg;
 	isc_timerevent_t *event = (isc_timerevent_t *)event0;
 
+	LOCK(&timer->lock);
 	if (ISC_LINK_LINKED(event, ev_timerlink)) {
 		/* The event was unlinked via timer_purge() */
 		timerevent_unlink(timer, event);
 	}
+	UNLOCK(&timer->lock);
 
 	isc_mem_put(timer->manager->mctx, event, event0->ev_size);
-	isc_timer_detach(&timer);
 }
 
 static void
@@ -215,8 +215,10 @@ timer_purge(isc_timer_t *timer) {
 	isc_timerevent_t *event = NULL;
 
 	while ((event = ISC_LIST_HEAD(timer->active)) != NULL) {
+		UNLOCK(&timer->lock);
 		bool purged = isc_task_purgeevent(timer->task,
 						  (isc_event_t *)event);
+		LOCK(&timer->lock);
 		if (!purged) {
 			/*
 			 * The event has already been executed, but not
@@ -227,25 +229,6 @@ timer_purge(isc_timer_t *timer) {
 	}
 }
 
-static void
-destroy(isc_timer_t *timer) {
-	isc_timermgr_t *manager = timer->manager;
-
-	LOCK(&manager->lock);
-
-	timer_purge(timer);
-	deschedule(timer);
-
-	UNLINK(manager->timers, timer, link);
-
-	UNLOCK(&manager->lock);
-
-	isc_task_detach(&timer->task);
-	isc_mutex_destroy(&timer->lock);
-	timer->magic = 0;
-	isc_mem_put(manager->mctx, timer, sizeof(*timer));
-}
-
 void
 isc_timer_create(isc_timermgr_t *manager, isc_task_t *task,
 		 isc_taskaction_t action, void *arg, isc_timer_t **timerp) {
@@ -272,8 +255,6 @@ isc_timer_create(isc_timermgr_t *manager, isc_task_t *task,
 		.arg = arg,
 	};
 
-	isc_refcount_init(&timer->references, 1);
-
 	isc_time_settoepoch(&timer->idle);
 
 	isc_task_attach(task, &timer->task);
@@ -378,30 +359,32 @@ isc_timer_gettype(isc_timer_t *timer) {
 }
 
 void
-isc_timer_attach(isc_timer_t *timer, isc_timer_t **timerp) {
-	REQUIRE(VALID_TIMER(timer));
-	REQUIRE(timerp != NULL && *timerp == NULL);
-	isc_refcount_increment(&timer->references);
+isc_timer_destroy(isc_timer_t **timerp) {
+	isc_timer_t *timer = NULL;
+	isc_timermgr_t *manager = NULL;
 
-	*timerp = timer;
-}
+	REQUIRE(timerp != NULL && VALID_TIMER(*timerp));
 
-void
-isc_timer_detach(isc_timer_t **timerp) {
-	isc_timer_t *timer;
-	/*
-	 * Detach *timerp from its timer.
-	 */
-
-	REQUIRE(timerp != NULL);
 	timer = *timerp;
-	REQUIRE(VALID_TIMER(timer));
-
-	if (isc_refcount_decrement(&timer->references) == 1) {
-		destroy(timer);
-	}
-
 	*timerp = NULL;
+
+	manager = timer->manager;
+
+	LOCK(&manager->lock);
+
+	LOCK(&timer->lock);
+	timer_purge(timer);
+	deschedule(timer);
+	UNLOCK(&timer->lock);
+
+	UNLINK(manager->timers, timer, link);
+
+	UNLOCK(&manager->lock);
+
+	isc_task_detach(&timer->task);
+	isc_mutex_destroy(&timer->lock);
+	timer->magic = 0;
+	isc_mem_put(manager->mctx, timer, sizeof(*timer));
 }
 
 static void
@@ -415,13 +398,13 @@ post_event(isc_timermgr_t *manager, isc_timer_t *timer, isc_eventtype_t type) {
 
 	ISC_LINK_INIT(event, ev_timerlink);
 	((isc_event_t *)event)->ev_destroy = timerevent_destroy;
-
-	isc_timer_attach(timer, &(isc_timer_t *){ NULL });
 	((isc_event_t *)event)->ev_destroy_arg = timer;
 
 	event->due = timer->due;
 
+	LOCK(&timer->lock);
 	ISC_LIST_APPEND(timer->active, event, ev_timerlink);
+	UNLOCK(&timer->lock);
 
 	isc_task_send(timer->task, ISC_EVENT_PTR(&event));
 }