Fix potential deadlock in TaskQueue's libevent PostTaskAndReply implementation

webrtc/base/task_queue_unittest.cc

 /*
  *  Copyright 2016 The WebRTC Project Authors. All rights reserved.
  *
  *  Use of this source code is governed by a BSD-style license
  *  that can be found in the LICENSE file in the root of the source
  *  tree. An additional intellectual property rights grant can be found
  *  in the file PATENTS.  All contributing project authors may
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
@@ skipping 25 matching lines @@
 }
 
 TEST(TaskQueueTest, PostAndCheckCurrent) {
   static const char kQueueName[] = "PostAndCheckCurrent";
   TaskQueue queue(kQueueName);
 
   // We're not running a task, so there shouldn't be a current queue.
   EXPECT_FALSE(queue.IsCurrent());
   EXPECT_FALSE(TaskQueue::Current());
 
   Event event(false, false);

[Taylor Brandstetter, 2017/02/22 00:24:27]

These tests still should put the Event above the TaskQueue, so that if
"event.Wait(1000)" fails, the event isn't accessed after being destroyed. Though
maybe, since the test will fail anyway, it doesn't matter if it crashes?

[tommi, 2017/02/22 12:43:54]

Ah, I was wondering why you moved the event objects in the other cl.
Good point.  If we crash, we could prevent other tests from running, so Done.

   queue.PostTask(Bind(&CheckCurrent, kQueueName, &event, &queue));
   EXPECT_TRUE(event.Wait(1000));
 }
 
 TEST(TaskQueueTest, PostCustomTask) {
   static const char kQueueName[] = "PostCustomImplementation";
   TaskQueue queue(kQueueName);
 
   Event event(false, false);
 
@@ skipping 142 matching lines @@
   TaskQueue reply_queue(kReplyQueue);
 
   Event event(false, false);
   bool my_flag = false;
   post_queue.PostTaskAndReply([&my_flag]() { my_flag = true; },
                               [&event]() { event.Set(); }, &reply_queue);
   EXPECT_TRUE(event.Wait(1000));
   EXPECT_TRUE(my_flag);
 }
 
+// This test covers a particular bug that we had in the libevent implementation
+// where we could hit a deadlock while trying to post a reply task to a queue
+// that was being deleted.  The test isn't guaranteed to hit that case but it's
+// written in a way that makes it likely and by running with --gtest_repeat=1000
+// the bug would occur. Alas, now it should be fixed.
+TEST(TaskQueueTest, PostAndReplyDeadlock) {
+  static const char kPostQueue[] = "PostQueue";

[Taylor Brandstetter, 2017/02/22 00:24:27]

Out of curiosity, what's the purpose of having "static const char kPostQueue[]"
in each test, as opposed to just 'TaskQueue post_queue("PostQueue");'?

[tommi, 2017/02/22 12:43:54]

When I started writing these tests, the constant variable name was the same
(kQueueName) but I changed its value from test to test and I checked that the
name of the queue was the same as the constant specified (just so that the
literal string would only be in one place).  Then that pattern was repeated as
more tests were added.  For many of these tests today, that's not necessary.

+  static const char kReplyQueue[] = "ReplyQueue";
+  std::unique_ptr<TaskQueue> post_queue(new TaskQueue(kPostQueue));

[Taylor Brandstetter, 2017/02/22 00:24:28]

These don't need to be unique_ptrs

[tommi, 2017/02/22 12:43:54]

Done.  I made them unique_ptrs while debugging so that I could make the deletion
explicit rather than implicit and be able to put a breakpoint on either the
.reset() or the line immediately following.

+  std::unique_ptr<TaskQueue> reply_queue(new TaskQueue(kReplyQueue));
+
+  Event event(false, false);
+  post_queue->PostTaskAndReply([&event]() { event.Set(); }, []() {},
+                               reply_queue.get());
+  EXPECT_TRUE(event.Wait(1000));
+  reply_queue.reset();
+}
+
 void TestPostTaskAndReply(TaskQueue* work_queue,
                           const char* work_queue_name,
                           Event* event) {
   ASSERT_FALSE(work_queue->IsCurrent());
   work_queue->PostTaskAndReply(
       Bind(&CheckCurrent, work_queue_name, nullptr, work_queue),
       NewClosure([event]() { event->Set(); }));
 }
 
 // Does a PostTaskAndReply from within a task to post and reply to the current
@@ skipping 33 matching lines @@
       queue.PostTask(NewClosure([&tasks_executed]() { ++tasks_executed; },
                                 [&tasks_cleaned_up]() { ++tasks_cleaned_up; }));
     event.Set();  // Unblock the first task.
   }
 
   EXPECT_GE(tasks_cleaned_up, tasks_executed);
   EXPECT_EQ(kTaskCount, tasks_cleaned_up);
 }
 
 }  // namespace rtc