Merge branch 'wpk/fix-taskmgr-pause-unpause-detach-race' into 'master'

Fix a race in taskmgr between worker and task pausing/unpausing.

Closes #1571

See merge request isc-projects/bind9!2918

CHANGES

@@ -1,3 +1,5 @@
+5349.	[bug]		Fix a race in task_pause/unpause. [GL #1571]
+
 5348.	[bug]		dnssec-settime -Psync was not being honoured.
 			[GL !2893]
 

lib/isc/task.c

@@ -78,13 +78,17 @@
  ***/
 
 typedef enum {
-	task_state_idle, task_state_ready, task_state_paused,
+	task_state_idle,	/* not doing anything, events queue empty */
-	task_state_running, task_state_done
+	task_state_ready,	/* waiting in worker's queue */
+	task_state_paused,	/* not running, paused */
+	task_state_pausing,	/* running, waiting to be paused */
+	task_state_running,	/* actively processing events */
+	task_state_done		/* shutting down, no events or references */
 } task_state_t;
 
 #if defined(HAVE_LIBXML2) || defined(HAVE_JSON_C)
 static const char *statenames[] = {
-	"idle", "ready", "running", "done",
+	"idle", "ready", "paused", "pausing", "running", "done",
 };
 #endif
 
@@ -381,6 +385,7 @@ task_shutdown(isc__task_t *task) {
 		}
 		INSIST(task->state == task_state_ready ||
 		       task->state == task_state_paused ||
+		       task->state == task_state_pausing ||
 		       task->state == task_state_running);
 
 		/*
@@ -502,7 +507,8 @@ task_send(isc__task_t *task, isc_event_t **eventp, int c) {
 	}
 	INSIST(task->state == task_state_ready ||
 	       task->state == task_state_running ||
-	       task->state == task_state_paused);
+	       task->state == task_state_paused ||
+	       task->state == task_state_pausing);
 	ENQUEUE(task->events, event, ev_link);
 	task->nevents++;
 	*eventp = NULL;
@@ -1200,10 +1206,12 @@ dispatch(isc__taskmgr_t *manager, unsigned int threadid) {
 						finished = true;
 						task->state = task_state_done;
 					} else {
-						/* It might be paused */
 						if (task->state ==
 						    task_state_running) {
 							task->state = task_state_idle;
+						} else if (task->state ==
+							   task_state_pausing) {
+							task->state = task_state_paused;
 						}
 					}
 					done = true;
@@ -1226,6 +1234,9 @@ dispatch(isc__taskmgr_t *manager, unsigned int threadid) {
 						 */
 						task->state = task_state_ready;
 						requeue = true;
+					} else if (task->state ==
+						   task_state_pausing) {
+						task->state = task_state_paused;
 					}
 					done = true;
 				}
@@ -1682,8 +1693,12 @@ isc_task_pause(isc_task_t *task0) {
 	INSIST(task->state == task_state_idle ||
 	       task->state == task_state_ready ||
 	       task->state == task_state_running);
-	running = (task->state == task_state_running);
+	if (task->state == task_state_running) {
-	task->state = task_state_paused;
+		running = true;
+		task->state = task_state_pausing;
+	} else {
+		task->state = task_state_paused;
+	}
 	UNLOCK(&task->lock);
 
 	if (running) {
@@ -1706,13 +1721,18 @@ isc_task_unpause(isc_task_t *task0) {
 	REQUIRE(ISCAPI_TASK_VALID(task0));
 
 	LOCK(&task->lock);
-	INSIST(task->state == task_state_paused);
+	INSIST(task->state == task_state_paused ||
-	if (!EMPTY(task->events)) {
+	       task->state == task_state_pausing);
-		task->state = task_state_ready;
+	/* If the task was pausing we can't reschedule it */
-		was_idle = true;
+	if (task->state == task_state_pausing) {
+		task->state = task_state_running;
 	} else {
 		task->state = task_state_idle;
 	}
+	if (task->state == task_state_idle && !EMPTY(task->events)) {
+		task->state = task_state_ready;
+		was_idle = true;
+	}
 	UNLOCK(&task->lock);
 
 	if (was_idle) {

lib/isc/tests/task_test.c

@@ -48,7 +48,7 @@ static isc_condition_t cv;
 
 atomic_int_fast32_t counter;
 static int active[10];
-static atomic_bool done;
+static atomic_bool done, done2;
 
 static int
 _setup(void **state) {
@@ -429,6 +429,83 @@ privilege_drop(void **state) {
 	assert_null(task2);
 }
 
+static void
+sleep_cb(isc_task_t *task, isc_event_t *event) {
+	UNUSED(task);
+	int p = *(int*)event->ev_arg;
+	if (p == 1) {
+		/*
+		 * Signal the main thread that we're running, so that
+		 * it can trigger the race.
+		 */
+		LOCK(&lock);
+		atomic_store(&done2, true);
+		SIGNAL(&cv);
+		UNLOCK(&lock);
+		/*
+		 * Wait for the operations in the main thread to be finished.
+		 */
+		LOCK(&lock);
+		while (!atomic_load(&done)) {
+			WAIT(&cv, &lock);
+		}
+		UNLOCK(&lock);
+	} else {
+		/*
+		 * Wait for the operations in the main thread to be finished.
+		 */
+		LOCK(&lock);
+		atomic_store(&done2, true);
+		SIGNAL(&cv);
+		UNLOCK(&lock);
+	}
+	isc_event_free(&event);
+}
+
+static void
+pause_unpause(void **state) {
+	isc_result_t result;
+	isc_task_t *task = NULL;
+	isc_event_t *event1,*event2 = NULL;
+	UNUSED(state);
+	atomic_store(&done, false);
+	atomic_store(&done2, false);
+
+	result = isc_task_create(taskmgr, 0, &task);
+	assert_int_equal(result, ISC_R_SUCCESS);
+
+	event1 = isc_event_allocate(test_mctx, task, ISC_TASKEVENT_TEST,
+				    sleep_cb, &(int){1}, sizeof (isc_event_t));
+	assert_non_null(event1);
+	event2 = isc_event_allocate(test_mctx, task, ISC_TASKEVENT_TEST,
+				    sleep_cb, &(int){2}, sizeof (isc_event_t));
+	assert_non_null(event2);
+	isc_task_send(task, &event1);
+	isc_task_send(task, &event2);
+	/* Wait for event1 to be running */
+	LOCK(&lock);
+	while (!atomic_load(&done2)) {
+		WAIT(&cv, &lock);
+	}
+	UNLOCK(&lock);
+	/* Pause-unpause-detach is what causes the race */
+	isc_task_pause(task);
+	isc_task_unpause(task);
+	isc_task_detach(&task);
+	/* Signal event1 to finish */
+	LOCK(&lock);
+	atomic_store(&done2, false);
+	atomic_store(&done, true);
+	SIGNAL(&cv);
+	UNLOCK(&lock);
+	/* Wait for event2 to finish */
+	LOCK(&lock);
+	while (!atomic_load(&done2)) {
+		WAIT(&cv, &lock);
+	}
+	UNLOCK(&lock);
+}
+
 /*
  * Basic task functions:
  */
@@ -1482,6 +1559,7 @@ main(int argc, char **argv) {
 		cmocka_unit_test_setup_teardown(purgeevent, _setup2, _teardown),
 		cmocka_unit_test_setup_teardown(purgeevent_notpurge,
 						_setup, _teardown),
+		cmocka_unit_test_setup_teardown(pause_unpause, _setup, _teardown),
 	};
 	int c;