Proposed ~TaskQueue() solution - GCD

webrtc/rtc_base/task_queue_gcd.cc

Index: webrtc/rtc_base/task_queue_gcd.cc
diff --git a/webrtc/rtc_base/task_queue_gcd.cc b/webrtc/rtc_base/task_queue_gcd.cc
index 45c5f09ebfc1f81a1ec18206bcd0c953f98579fb..3c1457f9b1acb62ccfff0cb8d90ec50a3d65119b 100644
--- a/webrtc/rtc_base/task_queue_gcd.cc
+++ b/webrtc/rtc_base/task_queue_gcd.cc
@@ -53,6 +53,10 @@ struct TaskQueue::QueueContext {
     delete qc;
   }
 
+  // By posting this function synchronously, one can wait until the queue
+  // has been emptied.
+  static void VoidFunction(void* context) {}
+
   TaskQueue* const queue;
   bool is_active;
 };
@@ -134,9 +138,15 @@ TaskQueue::~TaskQueue() {
   // queue have been released, the queue will be deallocated by the system.
   // This is why we check the context before running tasks.
 
-  // Use dispatch_sync to set the context to null to guarantee that there's not
-  // a race between checking the context and using it from a task.
-  dispatch_sync_f(queue_, context_, &QueueContext::SetNotActive);
+  // Toggle the context to not-active while the task-queue is suspended, to
+  // avoid a race.
+  dispatch_suspend(queue_);

[nisse-webrtc, 2017/08/30 09:20:35]

The interesting question is what dispatch_suspend does, if called concurrently
with a running task. If it blocks until the current task is finished, there
should be no race here.

[eladalon, 2017/08/30 09:41:27]

My thinking was that it would be helpful in either of these two possibilities:

* dispatch_suspend() blocks until the current task completes, and the next
pending task will not start executing until resume() is called.
* dispatch_suspend() actually pauses the currently running task.

However, some initial probing makes me suspect neither of these interpretations
is correct. Let me dig some more; I'll post an update later today.

[eladalon, 2017/08/30 10:21:42]

Okay, looks like it prevents the next task from launching, but given that the
current task might already be before/after the point where it checks the flag, I
don't see how suspend/resume helps in providing a better guarantee. We might as
well just call SetNotActive() directly.

+  QueueContext::SetNotActive(context_);
+  dispatch_resume(queue_);
+
+  // Wait until all previous tasks' destructors have been invoked.
+  dispatch_sync_f(queue_, context_, &QueueContext::VoidFunction);
+

[eladalon, 2017/08/30 09:19:36]

Might not hurt to add an RTC_DCHECK here to make sure nobody has posted any
additional tasks to the queue while we were waiting for VoidFunction() to be
executed; posting tasks to a TaskQueue while it's being torn down is an error.

   dispatch_release(queue_);
 }