Merge branch '1023-make-app.c-TSAN-clean' into 'master'

Make isc_app_t opaque and thread-safe

Closes #1023

See merge request !1936

CHANGES

+5235.	[cleanup]	Refactor lib/isc/app.c to be thread-safe, unused
+			parts of the API has been removed and the
+			isc_appctx_t data type has been changed to be
+			fully opaque. [GL #1023]
+
 5234.	[port]		arm: just use the compiler's default support for
 			yield. [GL #981]
 

bin/delv/delv.c

@@ -1622,9 +1622,9 @@ main(int argc, char *argv[]) {
 		fatal("failed to create mctx");
 
 	CHECK(isc_appctx_create(mctx, &actx));
-	CHECK(isc_taskmgr_createinctx(mctx, actx, 1, 0, &taskmgr));
-	CHECK(isc_socketmgr_createinctx(mctx, actx, &socketmgr));
-	CHECK(isc_timermgr_createinctx(mctx, actx, &timermgr));
+	CHECK(isc_taskmgr_createinctx(mctx, 1, 0, &taskmgr));
+	CHECK(isc_socketmgr_createinctx(mctx, &socketmgr));
+	CHECK(isc_timermgr_createinctx(mctx, &timermgr));
 
 	parse_args(argc, argv);
 

bin/rndc/rndc.c

@@ -278,7 +278,7 @@ rndc_senddone(isc_task_t *task, isc_event_t *event) {
 	if (sends == 0 && recvs == 0) {
 		isc_socket_detach(&sock);
 		isc_task_shutdown(task);
-		RUNTIME_CHECK(isc_app_shutdown() == ISC_R_SUCCESS);
+		isc_app_shutdown();
 	}
 }
 
@@ -347,7 +347,7 @@ rndc_recvdone(isc_task_t *task, isc_event_t *event) {
 	if (sends == 0 && recvs == 0) {
 		isc_socket_detach(&sock);
 		isc_task_shutdown(task);
-		RUNTIME_CHECK(isc_app_shutdown() == ISC_R_SUCCESS);
+		isc_app_shutdown();
 	}
 }
 

bin/tests/optional/shutdown_test.c

@@ -98,7 +98,7 @@ tick(isc_task_t *task, isc_event_t *event) {
 	info->ticks++;
 	if (strcmp(info->name, "1") == 0) {
 		if (info->ticks == 10) {
-			RUNTIME_CHECK(isc_app_shutdown() == ISC_R_SUCCESS);
+			isc_app_shutdown();
 		} else if (info->ticks >= 15 && info->exiting) {
 			isc_timer_detach(&info->timer);
 			isc_task_detach(&info->task);

configure.ac

@@ -635,10 +635,7 @@ LIBS="$PTHREAD_LIBS $LIBS"
 CFLAGS="$CFLAGS $PTHREAD_CFLAGS"
 CC="$PTHREAD_CC"
 
-#
-# We'd like to use sigwait() too
-#
-AC_CHECK_FUNCS([sigwait pthread_attr_getstacksize pthread_attr_setstacksize])
+AC_CHECK_FUNCS([pthread_attr_getstacksize pthread_attr_setstacksize])
 
 AC_ARG_WITH([locktype],
 	    AS_HELP_STRING([--with-locktype=ARG],

lib/dns/client.c

@@ -420,13 +420,13 @@ dns_client_create(dns_client_t **clientp, unsigned int options) {
 	result = isc_app_ctxstart(actx);
 	if (result != ISC_R_SUCCESS)
 		goto cleanup;
-	result = isc_taskmgr_createinctx(mctx, actx, 1, 0, &taskmgr);
+	result = isc_taskmgr_createinctx(mctx, 1, 0, &taskmgr);
 	if (result != ISC_R_SUCCESS)
 		goto cleanup;
-	result = isc_socketmgr_createinctx(mctx, actx, &socketmgr);
+	result = isc_socketmgr_createinctx(mctx, &socketmgr);
 	if (result != ISC_R_SUCCESS)
 		goto cleanup;
-	result = isc_timermgr_createinctx(mctx, actx, &timermgr);
+	result = isc_timermgr_createinctx(mctx, &timermgr);
 	if (result != ISC_R_SUCCESS)
 		goto cleanup;
 #if 0

lib/irs/context.c

@@ -101,15 +101,15 @@ ctxs_init(isc_mem_t **mctxp, isc_appctx_t **actxp,
 	if (result != ISC_R_SUCCESS)
 		goto fail;
 
-	result = isc_taskmgr_createinctx(*mctxp, *actxp, 1, 0, taskmgrp);
+	result = isc_taskmgr_createinctx(*mctxp, 1, 0, taskmgrp);
 	if (result != ISC_R_SUCCESS)
 		goto fail;
 
-	result = isc_socketmgr_createinctx(*mctxp, *actxp, socketmgrp);
+	result = isc_socketmgr_createinctx(*mctxp, socketmgrp);
 	if (result != ISC_R_SUCCESS)
 		goto fail;
 
-	result = isc_timermgr_createinctx(*mctxp, *actxp, timermgrp);
+	result = isc_timermgr_createinctx(*mctxp, timermgrp);
 	if (result != ISC_R_SUCCESS)
 		goto fail;
 

lib/isc/include/isc/app.h

@@ -103,14 +103,7 @@ typedef isc_event_t isc_appevent_t;
  * of the isc_app_ routines to work.  app implementations must maintain
  * all app context invariants.
  */
-struct isc_appctx {
-	unsigned int		impmagic;
-	unsigned int		magic;
-};
-
-#define ISCAPI_APPCTX_MAGIC		ISC_MAGIC('A','a','p','c')
-#define ISCAPI_APPCTX_VALID(c)		((c) != NULL && \
-					 (c)->magic == ISCAPI_APPCTX_MAGIC)
+struct isc_appctx;
 
 ISC_LANG_BEGINDECLS
 
@@ -184,10 +177,10 @@ isc_app_isrunning(void);
  *\li	false   App is not running.
  */
 
-isc_result_t
+void
 isc_app_ctxshutdown(isc_appctx_t *ctx);
 
-isc_result_t
+void
 isc_app_shutdown(void);
 /*!<
  * \brief Request application shutdown.
@@ -205,13 +198,13 @@ isc_app_shutdown(void);
  *\li	ISC_R_UNEXPECTED
  */
 
-isc_result_t
+void
 isc_app_ctxsuspend(isc_appctx_t *ctx);
 /*!<
  * \brief This has the same behavior as isc_app_ctxsuspend().
  */
 
-isc_result_t
+void
 isc_app_reload(void);
 /*!<
  * \brief Request application reload.
@@ -295,44 +288,6 @@ isc_appctx_destroy(isc_appctx_t **ctxp);
  *\li	*ctxp == NULL.
  */
 
-void
-isc_appctx_settaskmgr(isc_appctx_t *ctx, isc_taskmgr_t *taskmgr);
-/*!<
- * \brief Associate a task manager with an application context.
- *
- * This must be done before running tasks within the application context.
- *
- * Requires:
- *\li	'ctx' is a valid application context.
- *\li	'taskmgr' is a valid task manager.
- */
-
-void
-isc_appctx_setsocketmgr(isc_appctx_t *ctx, isc_socketmgr_t *socketmgr);
-/*!<
- * \brief Associate a socket manager with an application context.
- *
- * This must be done before handling socket events within the application
- * context.
- *
- * Requires:
- *\li	'ctx' is a valid application context.
- *\li	'socketmgr' is a valid socket manager.
- */
-
-void
-isc_appctx_settimermgr(isc_appctx_t *ctx, isc_timermgr_t *timermgr);
-/*!<
- * \brief Associate a socket timer with an application context.
- *
- * This must be done before handling timer events within the application
- * context.
- *
- * Requires:
- *\li	'ctx' is a valid application context.
- *\li	'timermgr' is a valid timer manager.
- */
-
 ISC_LANG_ENDDECLS
 
 #endif /* ISC_APP_H */

lib/isc/include/isc/socket.h

@@ -805,8 +805,7 @@ isc_socket_sendto2(isc_socket_t *sock, isc_region_t *region,
 /*@}*/
 
 isc_result_t
-isc_socketmgr_createinctx(isc_mem_t *mctx, isc_appctx_t *actx,
-			  isc_socketmgr_t **managerp);
+isc_socketmgr_createinctx(isc_mem_t *mctx, isc_socketmgr_t **managerp);
 
 isc_result_t
 isc_socketmgr_create(isc_mem_t *mctx, isc_socketmgr_t **managerp);

lib/isc/include/isc/task.h

@@ -630,7 +630,7 @@ isc_task_privilege(isc_task_t *task);
  *****/
 
 isc_result_t
-isc_taskmgr_createinctx(isc_mem_t *mctx, isc_appctx_t *actx,
+isc_taskmgr_createinctx(isc_mem_t *mctx,
 			unsigned int workers, unsigned int default_quantum,
 			isc_taskmgr_t **managerp);
 isc_result_t

lib/isc/include/isc/timer.h

@@ -316,8 +316,7 @@ isc_timer_gettype(isc_timer_t *timer);
  */
 
 isc_result_t
-isc_timermgr_createinctx(isc_mem_t *mctx, isc_appctx_t *actx,
-			 isc_timermgr_t **managerp);
+isc_timermgr_createinctx(isc_mem_t *mctx, isc_timermgr_t **managerp);
 
 isc_result_t
 isc_timermgr_create(isc_mem_t *mctx, isc_timermgr_t **managerp);

lib/isc/task.c

@@ -1871,7 +1871,7 @@ isc_taskmgr_renderjson(isc_taskmgr_t *mgr0, json_object *tasks) {
 
 
 isc_result_t
-isc_taskmgr_createinctx(isc_mem_t *mctx, isc_appctx_t *actx,
+isc_taskmgr_createinctx(isc_mem_t *mctx,
 			unsigned int workers, unsigned int default_quantum,
 			isc_taskmgr_t **managerp)
 {
@@ -1880,8 +1880,5 @@ isc_taskmgr_createinctx(isc_mem_t *mctx, isc_appctx_t *actx,
 	result = isc_taskmgr_create(mctx, workers, default_quantum,
 				       managerp);
 
-	if (result == ISC_R_SUCCESS)
-		isc_appctx_settaskmgr(actx, *managerp);
-
 	return (result);
 }

lib/isc/timer.c

@@ -784,15 +784,11 @@ isc_timermgr_destroy(isc_timermgr_t **managerp) {
 }
 
 isc_result_t
-isc_timermgr_createinctx(isc_mem_t *mctx, isc_appctx_t *actx,
-			 isc_timermgr_t **managerp)
+isc_timermgr_createinctx(isc_mem_t *mctx, isc_timermgr_t **managerp)
 {
 	isc_result_t result;
 
 	result = isc_timermgr_create(mctx, managerp);
 
-	if (result == ISC_R_SUCCESS)
-		isc_appctx_settimermgr(actx, *managerp);
-
 	return (result);
 }

lib/isc/unix/app.c

@@ -27,6 +27,7 @@
 #endif
 
 #include <isc/platform.h>
+#include <isc/atomic.h>
 #include <isc/app.h>
 #include <isc/condition.h>
 #include <isc/mem.h>
@@ -48,7 +49,7 @@
  * as an event loop dispatching various events.
  */
 static pthread_t		blockedthread;
-static bool			is_running;
+static atomic_bool		is_running;
 
 /*
  * The application context of this module.  This implementation actually
@@ -57,72 +58,41 @@ static bool			is_running;
 #define APPCTX_MAGIC		ISC_MAGIC('A', 'p', 'c', 'x')
 #define VALID_APPCTX(c)		ISC_MAGIC_VALID(c, APPCTX_MAGIC)
 
-typedef struct isc__appctx {
-	isc_appctx_t		common;
+struct isc_appctx {
+	unsigned int		magic;
 	isc_mem_t		*mctx;
 	isc_mutex_t		lock;
-	isc_eventlist_t		on_run;
-	bool		shutdown_requested;
-	bool		running;
-
-	/*!
-	 * We assume that 'want_shutdown' can be read and written atomically.
-	 */
-	bool		want_shutdown;
-	/*
-	 * We assume that 'want_reload' can be read and written atomically.
-	 */
-	bool		want_reload;
-
-	bool		blocked;
-
-	isc_taskmgr_t		*taskmgr;
-	isc_socketmgr_t		*socketmgr;
-	isc_timermgr_t		*timermgr;
+	isc_eventlist_t	on_run;
+	atomic_bool		shutdown_requested;
+	atomic_bool		running;
+	atomic_bool		want_shutdown;
+	atomic_bool		want_reload;
+	atomic_bool		blocked;
 	isc_mutex_t		readylock;
-	isc_condition_t		ready;
-} isc__appctx_t;
-
-static isc__appctx_t isc_g_appctx;
+	isc_condition_t	ready;
+};
 
-#ifndef HAVE_SIGWAIT
-static void
-exit_action(int arg) {
-	UNUSED(arg);
-	isc_g_appctx.want_shutdown = true;
-}
+static isc_appctx_t isc_g_appctx;
 
 static void
-reload_action(int arg) {
-	UNUSED(arg);
-	isc_g_appctx.want_reload = true;
-}
-#endif
-
-static isc_result_t
 handle_signal(int sig, void (*handler)(int)) {
 	struct sigaction sa;
-	char strbuf[ISC_STRERRORSIZE];
 
 	memset(&sa, 0, sizeof(sa));
 	sa.sa_handler = handler;
 
 	if (sigfillset(&sa.sa_mask) != 0 ||
 	    sigaction(sig, &sa, NULL) < 0) {
+		char strbuf[ISC_STRERRORSIZE];
 		strerror_r(errno, strbuf, sizeof(strbuf));
-		UNEXPECTED_ERROR(__FILE__, __LINE__,
+		isc_error_fatal(__FILE__, __LINE__,
 				 "handle_signal() %d setup: %s",
 				 sig, strbuf);
-		return (ISC_R_UNEXPECTED);
 	}
-
-	return (ISC_R_SUCCESS);
 }
 
 isc_result_t
-isc_app_ctxstart(isc_appctx_t *ctx0) {
-	isc__appctx_t *ctx = (isc__appctx_t *)ctx0;
-	isc_result_t result;
+isc_app_ctxstart(isc_appctx_t *ctx) {
 	int presult;
 	sigset_t sset;
 	char strbuf[ISC_STRERRORSIZE];
@@ -133,64 +103,27 @@ isc_app_ctxstart(isc_appctx_t *ctx0) {
 	 * Start an ISC library application.
 	 */
 
-	isc_mutex_init(&ctx->readylock);
+	isc_mutex_init(&ctx->lock);
 
+	isc_mutex_init(&ctx->readylock);
 	isc_condition_init(&ctx->ready);
 
-	isc_mutex_init(&ctx->lock);
-
 	ISC_LIST_INIT(ctx->on_run);
 
-	ctx->shutdown_requested = false;
-	ctx->running = false;
-	ctx->want_shutdown = false;
-	ctx->want_reload = false;
-	ctx->blocked = false;
-
-#ifndef HAVE_SIGWAIT
-	/*
-	 * Install do-nothing handlers for SIGINT and SIGTERM.
-	 *
-	 * We install them now because BSDI 3.1 won't block
-	 * the default actions, regardless of what we do with
-	 * pthread_sigmask().
-	 */
-	result = handle_signal(SIGINT, exit_action);
-	if (result != ISC_R_SUCCESS)
-		goto cleanup;
-	result = handle_signal(SIGTERM, exit_action);
-	if (result != ISC_R_SUCCESS)
-		goto cleanup;
-#endif
+	atomic_init(&ctx->shutdown_requested, false);
+	atomic_init(&ctx->running, false);
+	atomic_init(&ctx->want_shutdown, false);
+	atomic_init(&ctx->want_reload, false);
+	atomic_init(&ctx->blocked, false);
 
 	/*
 	 * Always ignore SIGPIPE.
 	 */
-	result = handle_signal(SIGPIPE, SIG_IGN);
-	if (result != ISC_R_SUCCESS)
-		goto cleanup;
+	handle_signal(SIGPIPE, SIG_IGN);
 
-	/*
-	 * On Solaris 2, delivery of a signal whose action is SIG_IGN
-	 * will not cause sigwait() to return. We may have inherited
-	 * unexpected actions for SIGHUP, SIGINT, and SIGTERM from our parent
-	 * process (e.g, Solaris cron).  Set an action of SIG_DFL to make
-	 * sure sigwait() works as expected.  Only do this for SIGTERM and
-	 * SIGINT if we don't have sigwait(), since a different handler is
-	 * installed above.
-	 */
-	result = handle_signal(SIGHUP, SIG_DFL);
-	if (result != ISC_R_SUCCESS)
-		goto cleanup;
-
-#ifdef HAVE_SIGWAIT
-	result = handle_signal(SIGTERM, SIG_DFL);
-	if (result != ISC_R_SUCCESS)
-		goto cleanup;
-	result = handle_signal(SIGINT, SIG_DFL);
-	if (result != ISC_R_SUCCESS)
-		goto cleanup;
-#endif
+	handle_signal(SIGHUP, SIG_DFL);
+	handle_signal(SIGTERM, SIG_DFL);
+	handle_signal(SIGINT, SIG_DFL);
 
 	/*
 	 * Block SIGHUP, SIGINT, SIGTERM.
@@ -206,61 +139,45 @@ isc_app_ctxstart(isc_appctx_t *ctx0) {
 	    sigaddset(&sset, SIGINT) != 0 ||
 	    sigaddset(&sset, SIGTERM) != 0) {
 		strerror_r(errno, strbuf, sizeof(strbuf));
-		UNEXPECTED_ERROR(__FILE__, __LINE__,
+		isc_error_fatal(__FILE__, __LINE__,
 				 "isc_app_start() sigsetops: %s", strbuf);
-		result = ISC_R_UNEXPECTED;
-		goto cleanup;
 	}
 	presult = pthread_sigmask(SIG_BLOCK, &sset, NULL);
 	if (presult != 0) {
 		strerror_r(presult, strbuf, sizeof(strbuf));
-		UNEXPECTED_ERROR(__FILE__, __LINE__,
+		isc_error_fatal(__FILE__, __LINE__,
 				 "isc_app_start() pthread_sigmask: %s",
 				 strbuf);
-		result = ISC_R_UNEXPECTED;
-		goto cleanup;
 	}
 
 	return (ISC_R_SUCCESS);
-
- cleanup:
-	(void)isc_condition_destroy(&ctx->ready);
-	(void)isc_mutex_destroy(&ctx->readylock);
-	return (result);
 }
 
 isc_result_t
 isc_app_start(void) {
-	isc_g_appctx.common.impmagic = APPCTX_MAGIC;
-	isc_g_appctx.common.magic = ISCAPI_APPCTX_MAGIC;
+	isc_g_appctx.magic = APPCTX_MAGIC;
 	isc_g_appctx.mctx = NULL;
 	/* The remaining members will be initialized in ctxstart() */
 
-	return (isc_app_ctxstart((isc_appctx_t *)&isc_g_appctx));
+	return (isc_app_ctxstart(&isc_g_appctx));
 }
 
 isc_result_t
 isc_app_onrun(isc_mem_t *mctx, isc_task_t *task, isc_taskaction_t action,
 	      void *arg)
 {
-	return (isc_app_ctxonrun((isc_appctx_t *)&isc_g_appctx, mctx,
-				  task, action, arg));
+	return (isc_app_ctxonrun(&isc_g_appctx, mctx, task, action, arg));
 }
 
 isc_result_t
-isc_app_ctxonrun(isc_appctx_t *ctx0, isc_mem_t *mctx, isc_task_t *task,
+isc_app_ctxonrun(isc_appctx_t *ctx, isc_mem_t *mctx, isc_task_t *task,
 		  isc_taskaction_t action, void *arg)
 {
-	isc__appctx_t *ctx = (isc__appctx_t *)ctx0;
 	isc_event_t *event;
 	isc_task_t *cloned_task = NULL;
-	isc_result_t result;
-
-	LOCK(&ctx->lock);
 
-	if (ctx->running) {
-		result = ISC_R_ALREADYRUNNING;
-		goto unlock;
+	if (atomic_load_acquire(&ctx->running)) {
+		return (ISC_R_ALREADYRUNNING);
 	}
 
 	/*
@@ -272,42 +189,34 @@ isc_app_ctxonrun(isc_appctx_t *ctx0, isc_mem_t *mctx, isc_task_t *task,
 				   action, arg, sizeof(*event));
 	if (event == NULL) {
 		isc_task_detach(&cloned_task);
-		result = ISC_R_NOMEMORY;
-		goto unlock;
+		return (ISC_R_NOMEMORY);
 	}
 
+	LOCK(&ctx->lock);
 	ISC_LIST_APPEND(ctx->on_run, event, ev_link);
-
-	result = ISC_R_SUCCESS;
-
- unlock:
 	UNLOCK(&ctx->lock);
 
-	return (result);
+	return (ISC_R_SUCCESS);
 }
 
 isc_result_t
-isc_app_ctxrun(isc_appctx_t *ctx0) {
-	isc__appctx_t *ctx = (isc__appctx_t *)ctx0;
-	int result;
+isc_app_ctxrun(isc_appctx_t *ctx) {
 	isc_event_t *event, *next_event;
 	isc_task_t *task;
 	sigset_t sset;
-	char strbuf[ISC_STRERRORSIZE];
-#ifdef HAVE_SIGWAIT
 	int sig;
-#endif /* HAVE_SIGWAIT */
+	bool exp_false = false;
+	bool exp_true = true;
 
 	REQUIRE(VALID_APPCTX(ctx));
 
-	LOCK(&ctx->lock);
-
-	if (!ctx->running) {
-		ctx->running = true;
-
+	if (atomic_compare_exchange_weak_acq_rel(
+		    &ctx->running, &exp_false, true) == true)
+	{
 		/*
 		 * Post any on-run events (in FIFO order).
 		 */
+		LOCK(&ctx->lock);
 		for (event = ISC_LIST_HEAD(ctx->on_run);
 		     event != NULL;
 		     event = next_event) {
@@ -317,17 +226,15 @@ isc_app_ctxrun(isc_appctx_t *ctx0) {
 			event->ev_sender = NULL;
 			isc_task_sendanddetach(&task, &event);
 		}
-
+		UNLOCK(&ctx->lock);
 	}
 
-	UNLOCK(&ctx->lock);
-
 	/*
 	 * BIND9 internal tools using multiple contexts do not
-	 * rely on signal.
-	 */
-	if (isc_bind9 && ctx != &isc_g_appctx)
+	 * rely on signal. */
+	if (isc_bind9 && ctx != &isc_g_appctx) {
 		return (ISC_R_SUCCESS);
+	}
 
 	/*
 	 * There is no danger if isc_app_shutdown() is called before we
@@ -335,8 +242,7 @@ isc_app_ctxrun(isc_appctx_t *ctx0) {
 	 * simply be made pending and we will get it when we call
 	 * sigwait().
 	 */
-	while (!ctx->want_shutdown) {
-#ifdef HAVE_SIGWAIT
+	while (atomic_load_acquire(&ctx->want_shutdown) == false) {
 		if (isc_bind9) {
 			/*
 			 * BIND9 internal; single context:
@@ -346,88 +252,56 @@ isc_app_ctxrun(isc_appctx_t *ctx0) {
 			    sigaddset(&sset, SIGHUP) != 0 ||
 			    sigaddset(&sset, SIGINT) != 0 ||
 			    sigaddset(&sset, SIGTERM) != 0) {
+				char strbuf[ISC_STRERRORSIZE];
 				strerror_r(errno, strbuf, sizeof(strbuf));
-				UNEXPECTED_ERROR(__FILE__, __LINE__,
-						 "isc_app_run() sigsetops: %s",
-						 strbuf);
-				return (ISC_R_UNEXPECTED);
+				isc_error_fatal(__FILE__, __LINE__,
+						"isc_app_run() sigsetops: %s",
+						strbuf);
 			}
 
-			result = sigwait(&sset, &sig);
-			if (result == 0) {
-				if (sig == SIGINT || sig == SIGTERM)
-					ctx->want_shutdown = true;
-				else if (sig == SIGHUP)
-					ctx->want_reload = true;
+			if (sigwait(&sset, &sig) == 0) {
+				switch (sig) {
+				case SIGINT:
+				case SIGTERM:
+					atomic_store_release(
+						&ctx->want_shutdown, true);
+					break;
+				case SIGHUP:
+					atomic_store_release(
+						&ctx->want_reload, true);
+					break;
+				default:
+					INSIST(0);