Introducing remoting::Stoppable helper base class implementing asynchronous shutdown on a specific t

Introducing remoting::Stoppable helper base class implementing asynchronous shutdown on a specific thread.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=149273

[alexeypa (please no reviews), 2012-07-23 20:50:24]

Please take a look.

[Sergey Ulanov, 2012-07-23 22:44:37]

It's not clear from CL description why we need it. Ref-counting MessageLoops and
Threads seems like a shady business and I think it's better to avoid it. Doesn't
MessageLoopProxy work the way it's supposed to work?

http://codereview.chromium.org/10796099/diff/1/remoting/base/auto_thread.cc
File remoting/base/auto_thread.cc (right):

http://codereview.chromium.org/10796099/diff/1/remoting/base/auto_thread.cc#n...
remoting/base/auto_thread.cc:19: thread_.Stop();
Ref-count may go to zero on any thread, but you can stop a thread only from the
thread that started it. This call will DCHECK() when a thread tries to commit
suicide. Its not possible to guarantee that we destroy everything properly if a
thread was allowed to stop itself like this.

http://codereview.chromium.org/10796099/diff/1/remoting/host/host_service_win.cc
File remoting/host/host_service_win.cc (right):

http://codereview.chromium.org/10796099/diff/1/remoting/host/host_service_win...
remoting/host/host_service_win.cc:201: new AutoThread(kIoThreadName,
message_loop_));
Why do we need thread to be refcounted?

Can we use base::SequencedWorkerPool instead as it is a preferred way to create
IO threads anyway?

http://codereview.chromium.org/10796099/diff/1/remoting/host/host_service_win...
remoting/host/host_service_win.cc:244: AutoMessageLoop
message_loop(MessageLoop::TYPE_UI);
You are creating ref-counted object on stack. That's never safe - the object
will try to delete itself when ref-count goes to zero.

http://codereview.chromium.org/10796099/diff/1/remoting/host/host_service_win...
remoting/host/host_service_win.cc:342: AutoMessageLoop
message_loop(MessageLoop::TYPE_DEFAULT);
Why do we need MessageLoop to be refcounted here? If we just need it to be
destroyed later than ServiceMain() returns (why?) then maybe allocate it on heap
and just leak it when we return?

[alexeypa (please no reviews), 2012-07-23 23:44:53]

> It's not clear from CL description why we need it. Ref-counting MessageLoops
and
> Threads seems like a shady business and I think it's better to avoid it.
Doesn't
> MessageLoopProxy work the way it's supposed to work?

MessageLoopProxy can outlive the message loop. This means that the shutdown
model is pretty much synchronous: one can try to post tasks but they are not
guaranteed to run. So the only reliable thigh to do is to free all resources
without trying to do anything. 

This would work in majority of scenarios except when we are required to post
tasks to shutdown at all. One example is destroying GUI windows on Windows. The
message loop has to be around until all windows are destroyed. So we have to
keep the message loop running for some time. Another example is when shutdown
has to happen on a different thread for whatever reason. Again, it means that
that other message loop has to be alive until then.

http://codereview.chromium.org/10796099/diff/1/remoting/base/auto_thread.cc
File remoting/base/auto_thread.cc (right):

http://codereview.chromium.org/10796099/diff/1/remoting/base/auto_thread.cc#n...
remoting/base/auto_thread.cc:19: thread_.Stop();
On 2012/07/23 22:44:37, sergeyu wrote:
> Ref-count may go to zero on any thread, but you can stop a thread only from
the
> thread that started it.

It is partially addressed. AutoThread is destroyed on |parent_| message loop.
See "parent_->DeleteSoon(FROM_HERE, this);" below. 

Now it is not enough. Start() and StartWithOptions() should also run on
|parent_| message loop. I'll fix that.

http://codereview.chromium.org/10796099/diff/1/remoting/host/host_service_win.cc
File remoting/host/host_service_win.cc (right):

http://codereview.chromium.org/10796099/diff/1/remoting/host/host_service_win...
remoting/host/host_service_win.cc:201: new AutoThread(kIoThreadName,
message_loop_));
On 2012/07/23 22:44:37, sergeyu wrote:
> Why do we need thread to be refcounted?

To make sure it is alive until all objects depending on it being live are gone.

> Can we use base::SequencedWorkerPool instead as it is a preferred way to
create
> IO threads anyway?

base::SequencedWorkerPool may be a better alternative. Let me see it is going to
work.

http://codereview.chromium.org/10796099/diff/1/remoting/host/host_service_win...
remoting/host/host_service_win.cc:244: AutoMessageLoop
message_loop(MessageLoop::TYPE_UI);
On 2012/07/23 22:44:37, sergeyu wrote:
> You are creating ref-counted object on stack. That's never safe - the object
> will try to delete itself when ref-count goes to zero.

I control the destruction logic for this object. AutoMessageLoop::OnDestruct()
will be called when the last reference goes away. It will stop the loop but do
not delete AutoMessageLoop.

Basically the use case for AutoMessageLoop and AutoThread is: the main loop is
create in the stack; all other loops are create in heap and use the main one to
destroy themselves; they a;so reference the main loop to make sure it stay alive
long enough.

http://codereview.chromium.org/10796099/diff/1/remoting/host/host_service_win...
remoting/host/host_service_win.cc:342: AutoMessageLoop
message_loop(MessageLoop::TYPE_DEFAULT);
On 2012/07/23 22:44:37, sergeyu wrote:
> Why do we need MessageLoop to be refcounted here? If we just need it to be
> destroyed later than ServiceMain() returns (why?) then maybe allocate it on
heap
> and just leak it when we return?

It doe snot have to outlive ServiceMain(). It is refcounted to express
dependencies between the message loop and objects using it. This refcounting has
nothing to do with memory allocation. The way those message loops are supposed
to be allocated is documented next to the definition of AutoThread and
AutoMessageLoop.

[Wez, 2012-07-24 10:57:40]

On 2012/07/23 20:50:24, alexeypa wrote:
> Please take a look.

Is there a design doc explaining the threading / object-lifetime model & usage
of these classes, or just the CL description & code comments? Either way, let's
discuss this when I'm back in Seattle?

The model I'm assuming for this stuff is:
* Some threads have lifetimes determined by the objects that use them; each
object holds a reference to the thread and when they are all gone, since by
definition nothing can be posted to the thread any more, it can exit.
* Objects that need to be torn down on a specific thread are also ref-counted,
and when the last reference is dropped they post a message to the thread to
delete themselves.

Is that basically what you have in mind here>

[alexeypa (please no reviews), 2012-07-24 15:53:50]

On 2012/07/24 10:57:40, Wez wrote:
> just the CL description & code comments?

Just CL description & code comments.

> * Some threads have lifetimes determined by the objects that use them; each
> object holds a reference to the thread and when they are all gone, since by
> definition nothing can be posted to the thread any more, it can exit.

Yes, pretty much. 

> * Objects that need to be torn down on a specific thread are also ref-counted,
> and when the last reference is dropped they post a message to the thread to
> delete themselves.

Object ownership is beyond the scope of this CL. It is up to the user of the
object to decide how memory is freed. All they get is a callback on the specific
thread.

Let's wait until you are back in Seattle. I'll probably split this CL into two:
one for AutoThread/AutoMessageLoop and the other one for Shutdownable.

[alexeypa (please no reviews), 2012-07-26 22:44:49]

I moved AutoThread and AutoMessageLoop to a separate CL. Please take another
look.

[alexeypa (please no reviews), 2012-07-27 20:11:11]

On 2012/07/26 22:44:49, alexeypa wrote:
> I moved AutoThread and AutoMessageLoop to a separate CL. Please take another
> look.

Ping.

[Sergey Ulanov, 2012-07-30 19:46:22]

mostly looks good. I just have some suggestions about shutdownable, mainly
regarding naming.

http://codereview.chromium.org/10796099/diff/15009/remoting/base/shutdownable.h
File remoting/base/shutdownable.h (right):

http://codereview.chromium.org/10796099/diff/15009/remoting/base/shutdownable...
remoting/base/shutdownable.h:18: // A wrapper implementing two-step shutdown for
an object.
suggest rewording it as "A helper base class that implements asynchronous
shutdown on a specific thread."

http://codereview.chromium.org/10796099/diff/15009/remoting/base/shutdownable...
remoting/base/shutdownable.h:19: class Shutdownable {
Maybe call it Stoppable? - it's easier to write and easier to read.

http://codereview.chromium.org/10796099/diff/15009/remoting/base/shutdownable...
remoting/base/shutdownable.h:21: typedef base::Callback<void(Shutdownable*)>
Callback;
Why not just use base::Closure? If users of that interface need to get reference
to the calling object they can always use currying when creating the callback.

http://codereview.chromium.org/10796099/diff/15009/remoting/base/shutdownable...
remoting/base/shutdownable.h:39: // logic.
Do we need this default implementation? If a class inherits from this class then
we can assume that it needs some stopping logic, so would it be better to make
this method pure virtual?

http://codereview.chromium.org/10796099/diff/15009/remoting/base/shutdownable...
remoting/base/shutdownable.h:45: kStopped
it should be either kShuttingDown/kShutdown or kStopping/kStopped. IMO it's
better to rename this interface to Stoppable and use kStopping/kStopped.

http://codereview.chromium.org/10796099/diff/15009/remoting/base/shutdownable...
remoting/base/shutdownable.h:48: State get_shutdownable_state() const { return
state_; }
drop get_, just shutdownable_state()

http://codereview.chromium.org/10796099/diff/15009/remoting/host/wts_session_...
File remoting/host/wts_session_process_launcher_win.cc (right):

http://codereview.chromium.org/10796099/diff/15009/remoting/host/wts_session_...
remoting/host/wts_session_process_launcher_win.cc:390: return;
add {} around this line for consistency?

http://codereview.chromium.org/10796099/diff/15009/remoting/host/wts_session_...
File remoting/host/wts_session_process_launcher_win.h (right):

http://codereview.chromium.org/10796099/diff/15009/remoting/host/wts_session_...
remoting/host/wts_session_process_launcher_win.h:52: const
Shutdownable::Callback& done,
done_callback?

[alexeypa (please no reviews), 2012-07-30 20:41:43]

Please take another look.

http://codereview.chromium.org/10796099/diff/15009/remoting/base/shutdownable.h
File remoting/base/shutdownable.h (right):

http://codereview.chromium.org/10796099/diff/15009/remoting/base/shutdownable...
remoting/base/shutdownable.h:18: // A wrapper implementing two-step shutdown for
an object.
On 2012/07/30 19:46:22, sergeyu wrote:
> suggest rewording it as "A helper base class that implements asynchronous
> shutdown on a specific thread."

Done.

http://codereview.chromium.org/10796099/diff/15009/remoting/base/shutdownable...
remoting/base/shutdownable.h:19: class Shutdownable {
On 2012/07/30 19:46:22, sergeyu wrote:
> Maybe call it Stoppable? - it's easier to write and easier to read.

Done.

http://codereview.chromium.org/10796099/diff/15009/remoting/base/shutdownable...
remoting/base/shutdownable.h:21: typedef base::Callback<void(Shutdownable*)>
Callback;
On 2012/07/30 19:46:22, sergeyu wrote:
> Why not just use base::Closure? If users of that interface need to get
reference
> to the calling object they can always use currying when creating the callback.

It was just a way to get |this| and pass a callback instance to the constructor.
I agree that base::Closure looks better.

http://codereview.chromium.org/10796099/diff/15009/remoting/base/shutdownable...
remoting/base/shutdownable.h:39: // logic.
On 2012/07/30 19:46:22, sergeyu wrote:
> Do we need this default implementation? If a class inherits from this class
then
> we can assume that it needs some stopping logic, so would it be better to make
> this method pure virtual?

Done.

http://codereview.chromium.org/10796099/diff/15009/remoting/base/shutdownable...
remoting/base/shutdownable.h:45: kStopped
On 2012/07/30 19:46:22, sergeyu wrote:
> it should be either kShuttingDown/kShutdown or kStopping/kStopped. IMO it's
> better to rename this interface to Stoppable and use kStopping/kStopped.

Done.

http://codereview.chromium.org/10796099/diff/15009/remoting/base/shutdownable...
remoting/base/shutdownable.h:48: State get_shutdownable_state() const { return
state_; }
On 2012/07/30 19:46:22, sergeyu wrote:
> drop get_, just shutdownable_state()

Done.

http://codereview.chromium.org/10796099/diff/15009/remoting/host/wts_session_...
File remoting/host/wts_session_process_launcher_win.cc (right):

http://codereview.chromium.org/10796099/diff/15009/remoting/host/wts_session_...
remoting/host/wts_session_process_launcher_win.cc:390: return;
On 2012/07/30 19:46:22, sergeyu wrote:
> add {} around this line for consistency?

Done.

http://codereview.chromium.org/10796099/diff/15009/remoting/host/wts_session_...
File remoting/host/wts_session_process_launcher_win.h (right):

http://codereview.chromium.org/10796099/diff/15009/remoting/host/wts_session_...
remoting/host/wts_session_process_launcher_win.h:52: const
Shutdownable::Callback& done,
On 2012/07/30 19:46:22, sergeyu wrote:
> done_callback?

I called it |stopped_callback|

[alexeypa (please no reviews), 2012-07-31 17:48:30]

Ping.

[Sergey Ulanov, 2012-07-31 17:50:45]

lgtm. Sorry for delay.

[alexeypa (please no reviews), 2012-07-31 17:55:08]

On 2012/07/31 17:50:45, sergeyu wrote:
> lgtm. Sorry for delay.

Not a problem. I know you were busy yesterday. Thanks!

[commit-bot: I haz the power, 2012-07-31 17:57:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alexeypa@chromium.org/10796099/23003

[commit-bot: I haz the power, 2012-07-31 18:16:46]

Try job failure for 10796099-23003 (retry) on mac_rel for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-07-31 18:32:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alexeypa@chromium.org/10796099/14005

[commit-bot: I haz the power, 2012-07-31 18:56:03]

Try job failure for 10796099-14005 (retry) on linux_chromeos for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2012-07-31 18:59:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alexeypa@chromium.org/10796099/20017

[commit-bot: I haz the power, 2012-07-31 19:40:29]

Try job failure for 10796099-20017 (previous was lost) (retry) (retry) on
mac_rel for steps "base_unittests, browser_tests, cacheinvalidation_unittests,
jingle_unittests, remoting_unittests, interactive_ui_tests, crypto_unittests,
net_unittests, unit_tests, gpu_unittests, nacl_integration, media_unittests,
safe_browsing_tests, ipc_tests, printing_unittests, sql_unittests,
content_unittests, sync_unit_tests".
It's a second try, previously, steps "base_unittests, browser_tests,
cacheinvalidation_unittests, jingle_unittests, remoting_unittests,
interactive_ui_tests, crypto_unittests, net_unittests, unit_tests,
gpu_unittests, nacl_integration, media_unittests, safe_browsing_tests,
ipc_tests, printing_unittests, sql_unittests, content_unittests,
sync_unit_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-07-31 20:01:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alexeypa@chromium.org/10796099/20017

[commit-bot: I haz the power, 2012-07-31 21:06:28]

Change committed as 149273

[Wez, 2012-08-18 01:08:29]

https://chromiumcodereview.appspot.com/10796099/diff/20017/remoting/base/stop...
File remoting/base/stoppable.h (right):

https://chromiumcodereview.appspot.com/10796099/diff/20017/remoting/base/stop...
remoting/base/stoppable.h:24: explicit
Stoppable(scoped_refptr<base::SingleThreadTaskRunner> task_runner,
This doesn't need to be explicit, does it?

[alexeypa (please no reviews), 2012-08-18 01:25:35]

https://chromiumcodereview.appspot.com/10796099/diff/20017/remoting/base/stop...
File remoting/base/stoppable.h (right):

https://chromiumcodereview.appspot.com/10796099/diff/20017/remoting/base/stop...
remoting/base/stoppable.h:24: explicit
Stoppable(scoped_refptr<base::SingleThreadTaskRunner> task_runner,
On 2012/08/18 01:08:29, Wez wrote:
> This doesn't need to be explicit, does it?

Yes, you are right. It should not be explicit. I'll fix it in one of the
upcoming CLs.