[Chromoting] Introducing refcount-based life time management of the message loops in the service (d…

remoting/base/auto_thread_task_runner.h

+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef REMOTING_BASE_AUTO_THREAD_TASK_RUNNER_H_
+#define REMOTING_BASE_AUTO_THREAD_TASK_RUNNER_H_
+
+#include "base/basictypes.h"
+#include "base/callback.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/message_loop.h"
+#include "base/single_thread_task_runner.h"
+
+class MessageLoop;
+
+namespace remoting {
+
+// This class provides automatic life time management for
+// |SingleThreadTaskRunner|. The task runner is stopped when the last reference
+// to |AutoThreadTaskRunner| is dropped.

[Wez, 2012/08/30 23:56:43]

This still isn't true, though; the task runner is _not_ stopped when the last
reference is dropped - it may be stopped by the calling code calling Stop() on
it if it's a Thread, or by the calling code providing a |stop_callback| that
stops it, but this class doesn't actually stop it.

Suggest rewording:
"This class supports automatic lifetime management for SingleThreadTaskRunners,
by invoking a caller-supplied callback when no more references remain, that can
stop the underlying thread or message loop, n. The caller may also provide a
reference to a parent AutoThreadTaskRunner to express dependencies between child
and parent thread lifetimes."

[alexeypa (please no reviews), 2012/08/31 19:57:36]

Done.

+class AutoThreadTaskRunner : public base::SingleThreadTaskRunner {

[Sergey Ulanov, 2012/08/31 01:25:18]

can you please add some unittests for this class?

[alexeypa (please no reviews), 2012/08/31 19:57:36]

Done.

+ public:
+  // Constructs an instance of |AutoThreadTaskRunner| wrapping |task_runner|.
+  // |stop_callback| is called (on arbitraty thread) when the last reference to
+  // the object is dropped.
+  explicit AutoThreadTaskRunner(
+      scoped_refptr<base::SingleThreadTaskRunner> task_runner);
+  AutoThreadTaskRunner(scoped_refptr<base::SingleThreadTaskRunner> task_runner,
+                       const base::Closure& stop_callback);
+
+  // TODO(alexeypa): Remove the |parent| reference once Thread class supports
+  // stopping the thread's message loop and joining the thread separately.

[Wez, 2012/08/30 23:56:43]

nit: Add a reference to crbug.com/145856 here?

[alexeypa (please no reviews), 2012/08/31 19:57:36]

Done.

+  //
+  // Background: currently the only legitimate way of stopping a thread is to
+  // call Thread::Stop() from the same thread that started it. Thread::Stop()
+  // will stop the thread message loop by posting a private task and join
+  // the thread. Thread::Stop() verifies that it is called from the right thread
+  // and that the message loop has been stopped by the private task mentioned
+  // above.

[Wez, 2012/08/30 23:56:43]

nit: You only need the first sentence of this comment, followed immediately by
the explanation of the |parent| relationship below.

[alexeypa (please no reviews), 2012/08/31 19:57:36]

I think it deserves a verbose comment, given the number of questions asked about
this specific behavior. 

PS. It also explains why we cannot just call Thread::Stop from the callback.

+  //
+  // Until Thread::Stop() is split in two separate operations, it should not be
+  // called while any objects use it. This is achieved by keeping a reference to
+  // the parent thread's task runner. Since Thread::Stop() is called after
+  // exiting the parent loop, keeping a reference to it gives enough guaratee.

[Wez, 2012/08/30 23:56:43]

typo: guarantee

[alexeypa (please no reviews), 2012/08/31 19:57:36]

Done.

+  // |parent| should itself be an AutoThreadTaskRunner managed thread, to
+  // guarantee that it remains alive long enough to perform the Stop().

[Wez, 2012/08/30 23:56:43]

Suggest rewording:
"To work around this we require that the parent thread of an
AutoThreadTaskRunner-wrapped Thread provide an AutoThreadTaskRunner so it can
tell when all its child Threads are finished with and Stop() them."

[alexeypa (please no reviews), 2012/08/31 19:57:36]

This sounds confusing to me. Check out my variant.

+  AutoThreadTaskRunner(scoped_refptr<base::SingleThreadTaskRunner> task_runner,
+                       scoped_refptr<base::SingleThreadTaskRunner> parent);

[Wez, 2012/08/30 23:56:43]

Do we ever need to pass a non-AutoThreadTaskRunner |parent| for our current
workaround? If not then perhaps we should restrict |parent| to be an
AutoThreadTaskRunner.

[alexeypa (please no reviews), 2012/08/31 19:57:36]

Done.

+  AutoThreadTaskRunner(scoped_refptr<base::SingleThreadTaskRunner> task_runner,
+                       scoped_refptr<base::SingleThreadTaskRunner> parent,
+                       const base::Closure& stop_callback);
+
+  // SingleThreadTaskRunner implementation
+  virtual bool PostDelayedTask(const tracked_objects::Location& from_here,
+                               const base::Closure& task,
+                               base::TimeDelta delay) OVERRIDE;
+  virtual bool PostNonNestableDelayedTask(
+      const tracked_objects::Location& from_here,
+      const base::Closure& task,
+      base::TimeDelta delay) OVERRIDE;
+  virtual bool RunsTasksOnCurrentThread() const OVERRIDE;
+
+ private:
+  friend class base::DeleteHelper<AutoThreadTaskRunner>;
+
+  virtual ~AutoThreadTaskRunner();
+
+  // An reference to the parent message loop to keep it alive while
+  // |task_runner_| is running.
+  scoped_refptr<base::SingleThreadTaskRunner> parent_;
+
+  // This callback quits |task_runner_|. It can be run on any thread.
+  base::Closure stop_callback_;
+
+  // The wrapped task runner.
+  scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
+
+  DISALLOW_COPY_AND_ASSIGN(AutoThreadTaskRunner);
+};
+
+}  // namespace remoting
+
+#endif  // REMOTING_BASE_AUTO_THREAD_TASK_RUNNER_H_