Allow SequencedTaskRunnerHandle::Get() while running unsequenced tasks.

base/threading/sequenced_worker_pool_unittest.cc

Index: base/threading/sequenced_worker_pool_unittest.cc
diff --git a/base/threading/sequenced_worker_pool_unittest.cc b/base/threading/sequenced_worker_pool_unittest.cc
index 5812ee75ac5f84df1fc7df48d107b1a1c69d84e8..d33ff6266613be39c72bbe924b4a79f4e1550eaa 100644
--- a/base/threading/sequenced_worker_pool_unittest.cc
+++ b/base/threading/sequenced_worker_pool_unittest.cc
@@ -13,6 +13,7 @@
 #include "base/message_loop/message_loop.h"
 #include "base/synchronization/condition_variable.h"
 #include "base/synchronization/lock.h"
+#include "base/synchronization/waitable_event.h"
 #include "base/test/sequenced_task_runner_test_template.h"
 #include "base/test/sequenced_worker_pool_owner.h"
 #include "base/test/task_runner_test_template.h"
@@ -904,47 +905,67 @@ TEST_F(SequencedWorkerPoolTest, FlushForTesting) {
   pool()->FlushForTesting();
 }
 
-namespace {
-
-void CheckWorkerPoolAndSequenceToken(
-    const scoped_refptr<SequencedWorkerPool>& expected_pool,
-    SequencedWorkerPool::SequenceToken expected_token) {
-  SequencedWorkerPool::SequenceToken token =
-      SequencedWorkerPool::GetSequenceTokenForCurrentThread();
-  EXPECT_EQ(expected_token.ToString(), token.ToString());
-
-  scoped_refptr<SequencedWorkerPool> pool =
-      SequencedWorkerPool::GetWorkerPoolForCurrentThread();
-  EXPECT_EQ(expected_pool, pool);
+// Helper method for VerifyCurrentSequencedTaskRunner().
+void VerifySequencedTaskRunnerRunsOnCurrentThread(
+    const scoped_refptr<SequencedTaskRunner>& task_runner,

[danakj, 2015/11/17 22:03:39]

nit: take smart pointers by value if you want to hold a reference. take T* if
you don't want a reference.

i think you want SequencedTaskRunner* here?

[Bernhard Bauer, 2015/11/18 13:15:50]

Done.

+    bool should_run_on_current_thread,
+    const Closure& callback) {
+  EXPECT_EQ(should_run_on_current_thread,
+            task_runner->RunsTasksOnCurrentThread());
+  callback.Run();
 }
 
-}  // namespace
-
-TEST_F(SequencedWorkerPoolTest, GetWorkerPoolAndSequenceTokenForCurrentThread) {
-  EnsureAllWorkersCreated();
+void VerifyCurrentSequencedTaskRunner(
+    scoped_refptr<SequencedTaskRunner> expected_task_runner,

[danakj, 2015/11/17 22:03:39]

and here?

[Bernhard Bauer, 2015/11/18 13:15:50]

But here I do want to take a reference (and then pass it to the closure below),
no?

[danakj, 2015/11/20 19:18:43]

You don't need a ref here to pass to the closure. The caller of this is waiting
until the |callback| is run, at which point these will not be referring to the
task runner any more. This could totally be a T* (which was why I wanted you to
pass a T* in the Bind() of this function).

However.. I noticed that on line 928 you're getting a scoped_refptr back from
GetSTRForCurrentThread(). There's nothing keeping that one alive, so I guess you
do actually need to pass a scoped_refptr. Given that, you should probably undo
passing a raw pointer to the Bind of this function, cuz it's just going into a
scoped_refptr anyways. Sorry for being confused here.

+    bool expected_equal,
+    const Closure& callback) {
+  scoped_refptr<SequencedTaskRunner> task_runner =
+      SequencedWorkerPool::GetSequencedTaskRunnerForCurrentThread();
 
-  // The current thread should have neither a worker pool nor a sequence token.
-  SequencedWorkerPool::SequenceToken local_token =
-      SequencedWorkerPool::GetSequenceTokenForCurrentThread();
-  scoped_refptr<SequencedWorkerPool> local_pool =
-      SequencedWorkerPool::GetWorkerPoolForCurrentThread();
-  EXPECT_FALSE(local_token.IsValid()) << local_token.ToString();
-  EXPECT_FALSE(local_pool);
+  // If the expected task runner is null, get another one for the current
+  // thread. They should be the same.
+  if (!expected_task_runner) {
+    expected_task_runner =
+        SequencedWorkerPool::GetSequencedTaskRunnerForCurrentThread();
+  }
 
-  SequencedWorkerPool::SequenceToken token1 = pool()->GetSequenceToken();
-  SequencedWorkerPool::SequenceToken token2 = pool()->GetSequenceToken();
-  pool()->PostSequencedWorkerTask(
-      token1, FROM_HERE,
-      base::Bind(&CheckWorkerPoolAndSequenceToken, pool(), token1));
-  pool()->PostSequencedWorkerTask(
-      token2, FROM_HERE,
-      base::Bind(&CheckWorkerPoolAndSequenceToken, pool(), token2));
+  // SequencedTaskRunner does not allow directly checking for equality, but we
+  // can post a task to one task runner and verify that the other task runner
+  // is on the same sequence.
+  task_runner->PostTask(FROM_HERE,
+                        Bind(&VerifySequencedTaskRunnerRunsOnCurrentThread,
+                             expected_task_runner, expected_equal, callback));

[danakj, 2015/11/17 22:03:39]

nit: move() the task runner smart pointer, no need to keep a ref here.

but probably you don't want a refptr at all

[Bernhard Bauer, 2015/11/18 13:15:50]

Moving the scoped_refptr (see above).

+}
 
-  pool()->PostWorkerTask(FROM_HERE,
-                         base::Bind(&CheckWorkerPoolAndSequenceToken, pool(),
-                                    SequencedWorkerPool::SequenceToken()));
+TEST_F(SequencedWorkerPoolTest, GetSequencedTaskRunnerForCurrentThread) {
+  EnsureAllWorkersCreated();
 
-  pool()->FlushForTesting();
+  // The current thread should not have a sequenced task runner from a
+  // worker pool.
+  scoped_refptr<SequencedTaskRunner> local_task_runner =
+      SequencedWorkerPool::GetSequencedTaskRunnerForCurrentThread();
+  EXPECT_FALSE(local_task_runner);
+
+  WaitableEvent event(false, false);
+  Closure signal = Bind(&WaitableEvent::Signal, Unretained(&event));
+  scoped_refptr<SequencedTaskRunner> task_runner_1 =
+      pool()->GetSequencedTaskRunner(SequencedWorkerPool::GetSequenceToken());
+  scoped_refptr<SequencedTaskRunner> task_runner_2 =
+      pool()->GetSequencedTaskRunner(SequencedWorkerPool::GetSequenceToken());
+  task_runner_1->PostTask(FROM_HERE, Bind(&VerifyCurrentSequencedTaskRunner,
+                                          task_runner_1, true, signal));

[danakj, 2015/11/17 22:03:39]

.get() on all these, to pass the runner pointer. we'll wait for the tasks to run
on the next line so no need to introduce ref counting.

[Bernhard Bauer, 2015/11/18 13:15:50]

Apparently, if the first argument to a Closure is refcounted, it needs to be in
a scoped_refptr, otherwise I get a static assertion failure
(https://code.google.com/p/chromium/codesearch#chromium/src/base/bind.h&sq=pac...).

[danakj, 2015/11/18 19:06:31]

Unless you use base::Unretained, I thought?

[gab, 2015/11/18 22:37:33]

Right.

[Bernhard Bauer, 2015/11/19 10:15:12]

So... you want me to do that then? Done (although given the choice I personally
would prefer to trade the readability of using |task_runner_n| as it is in
exchange for the overhead of refcounting).

[danakj, 2015/11/19 18:49:07]

Yes please, it's not the overhead I'm worried about but "own it in as few places
as possible". So if there's no reason to pass refs around then don't.

[danakj, 2015/11/20 19:18:43]

I was wrong :( Didn't see the GetSTRForCurrentThread(), which does need to be
kept alive. Simplest way to do that is to pass a refptr here too.

+  event.Wait();
+  task_runner_2->PostTask(FROM_HERE, Bind(&VerifyCurrentSequencedTaskRunner,
+                                          task_runner_2, true, signal));
+  event.Wait();
+
+  task_runner_1->PostTask(FROM_HERE, Bind(&VerifyCurrentSequencedTaskRunner,
+                                          task_runner_2, false, signal));
+  event.Wait();
+
+  pool()->PostWorkerTask(FROM_HERE, Bind(&VerifyCurrentSequencedTaskRunner,
+                                         nullptr, true, signal));
+  event.Wait();
 }
 
 TEST(SequencedWorkerPoolRefPtrTest, ShutsDownCleanWithContinueOnShutdown) {