Refactor ArcBridgeServiceImpl part 1.

components/arc/arc_bridge_service_impl.cc

 // Copyright 2015 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.
 
 #include "components/arc/arc_bridge_service_impl.h"
 
 #include <string>
 #include <utility>
 
 #include "base/command_line.h"
 #include "base/json/json_writer.h"
 #include "base/sequenced_task_runner.h"
 #include "base/sys_info.h"
 #include "base/task_runner_util.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "base/time/time.h"
 #include "chromeos/chromeos_switches.h"
 #include "chromeos/dbus/dbus_method_call_status.h"
 #include "chromeos/dbus/dbus_thread_manager.h"
 #include "components/arc/arc_session.h"
 #include "components/prefs/pref_registry_simple.h"
 #include "components/prefs/pref_service.h"
 
 namespace arc {
 namespace {

[Luis Héctor Chávez, 2016/12/16 23:02:13]

nit: maybe add a newline before for consistency?

[hidehiko, 2016/12/19 07:41:21]

Done.

-constexpr int64_t kReconnectDelayInSeconds = 5;
+
+constexpr base::TimeDelta kDefaultRestartDelay =
+    base::TimeDelta::FromSeconds(5);
+
 }  // namespace
 
 ArcBridgeServiceImpl::ArcBridgeServiceImpl(
     const scoped_refptr<base::TaskRunner>& blocking_task_runner)
     : session_started_(false),
+      restart_delay_(kDefaultRestartDelay),
       factory_(base::Bind(ArcSession::Create, this, blocking_task_runner)),
       weak_factory_(this) {}
 
 ArcBridgeServiceImpl::~ArcBridgeServiceImpl() {
   if (arc_session_)
     arc_session_->RemoveObserver(this);
 }
 
 void ArcBridgeServiceImpl::RequestStart() {
   DCHECK(CalledOnValidThread());
   if (session_started_)
     return;
   VLOG(1) << "Session started";
   session_started_ = true;
   PrerequisitesChanged();
 }
 
 void ArcBridgeServiceImpl::RequestStop() {
   DCHECK(CalledOnValidThread());
   if (!session_started_)
     return;
   VLOG(1) << "Session ended";
   session_started_ = false;
   PrerequisitesChanged();
 }
 
 void ArcBridgeServiceImpl::OnShutdown() {
   DCHECK(CalledOnValidThread());
+
   VLOG(1) << "OnShutdown";
-  if (!session_started_)
-    return;
   session_started_ = false;
-  reconnect_ = false;
+  restart_timer_.Stop();
   if (arc_session_)
     arc_session_->OnShutdown();
+  // ArcSession::OnShutdown() invokes OnSessionStopped() synchronously.
+  // In the observer method, |arc_session_| should be destroyed.
+  DCHECK(!arc_session_);
 }
 
 void ArcBridgeServiceImpl::SetArcSessionFactoryForTesting(
     const ArcSessionFactory& factory) {
   DCHECK(!factory.is_null());
   factory_ = factory;
 }
 
-void ArcBridgeServiceImpl::DisableReconnectDelayForTesting() {
-  use_delay_before_reconnecting_ = false;
+void ArcBridgeServiceImpl::SetRestartDelayForTesting(
+    const base::TimeDelta& restart_delay) {
+  DCHECK_EQ(state(), State::STOPPED);
+  DCHECK(!arc_session_);
+  DCHECK(!restart_timer_.IsRunning());
+  restart_delay_ = restart_delay;
 }
 
 void ArcBridgeServiceImpl::PrerequisitesChanged() {

[Luis Héctor Chávez, 2016/12/16 23:02:13]

Now that you've removed the third caller of PrerequisitesChanged(), how about
inlining the correct half of the method in the two places where it's called?

[Luis Héctor Chávez, 2016/12/16 23:06:35]

Nevermind about this, I just saw your second patch where you actually do this.

[hidehiko, 2016/12/19 07:41:21]

Yes. Let me work on it in the following CL.

   DCHECK(CalledOnValidThread());
   VLOG(1) << "Prerequisites changed. "
           << "state=" << static_cast<uint32_t>(state())
           << ", session_started=" << session_started_;
   if (state() == State::STOPPED) {
-    if (!session_started_)
+    if (!session_started_) {
+      // We were explicitly asked to stop, so do not restart.
+      restart_timer_.Stop();

[Luis Héctor Chávez, 2016/12/16 23:02:13]

How about you move this to L96-97 to RequestStop(), where it makes more sense?

[hidehiko, 2016/12/19 07:41:21]

If this is not a big problem, can I keep this in this CL.
In this CL, I'd like to keep the main implementation of starting/stopping in
this method, which looks the current implementation direction to me.

Instead, the code around here is largely refactored in the following CL, and I'd
like to address your comments in the CL.

       return;
+    }
+    DCHECK(!restart_timer_.IsRunning());

[Luis Héctor Chávez, 2016/12/16 23:02:13]

Both branches check for this. Move to after L93?

[hidehiko, 2016/12/19 07:41:21]

Similar to above, can I address this in the following CL?

     VLOG(0) << "Prerequisites met, starting ARC";
-    SetStopReason(StopReason::SHUTDOWN);
-
-    if (arc_session_)
-      arc_session_->RemoveObserver(this);
-
-    SetState(State::CONNECTING);
-    arc_session_ = factory_.Run();
-    arc_session_->AddObserver(this);
-    arc_session_->Start();
+    StartArcSession();
   } else {
+    DCHECK(!restart_timer_.IsRunning());
     if (session_started_)
       return;
     VLOG(0) << "Prerequisites stopped being met, stopping ARC";
     StopInstance();
   }
 }
 
+void ArcBridgeServiceImpl::StartArcSession() {
+  DCHECK(CalledOnValidThread());
+  DCHECK_EQ(state(), State::STOPPED);
+  DCHECK(!arc_session_);
+  DCHECK(!restart_timer_.IsRunning());
+
+  VLOG(1) << "Starting ARC instance";
+  SetStopReason(StopReason::SHUTDOWN);
+  arc_session_ = factory_.Run();
+  arc_session_->AddObserver(this);
+  SetState(State::CONNECTING);
+  arc_session_->Start();
+}
+
 void ArcBridgeServiceImpl::StopInstance() {
   DCHECK(CalledOnValidThread());
-  if (state() == State::STOPPED || state() == State::STOPPING) {
+  DCHECK_NE(state(), State::STOPPED);
+  DCHECK(!restart_timer_.IsRunning());
+
+  if (state() == State::STOPPING) {
     VLOG(1) << "StopInstance() called when ARC is not running";
     return;
   }
 
-  // We were explicitly asked to stop, so do not reconnect.
-  reconnect_ = false;
-
   VLOG(1) << "Stopping ARC";
   DCHECK(arc_session_.get());
   SetState(State::STOPPING);
 
   // Note: this can call OnStopped() internally as a callback.
   arc_session_->Stop();
 }
 
 void ArcBridgeServiceImpl::OnSessionReady() {
   DCHECK(CalledOnValidThread());
   if (state() != State::CONNECTING) {
     VLOG(1) << "StopInstance() called while connecting";
     return;
   }
 
-  // The container can be considered to have been successfully launched, so
-  // restart if the connection goes down without being requested.
-  reconnect_ = true;
   VLOG(0) << "ARC ready";
   SetState(State::READY);
 }
 
 void ArcBridgeServiceImpl::OnSessionStopped(StopReason stop_reason) {
   DCHECK(CalledOnValidThread());
+  DCHECK(!restart_timer_.IsRunning());
+
   VLOG(0) << "ARC stopped: " << stop_reason;
   arc_session_->RemoveObserver(this);
   arc_session_.reset();
+
+  // If READY, ARC instance is unexpectedly crashed so we need to restart it

[Luis Héctor Chávez, 2016/12/16 23:02:13]

nit: "ARC instance unexpectedly crashed".

[hidehiko, 2016/12/19 07:41:21]

Done.

+  // automatically. If STOPPING, it is the result of consecutive RequestStop()
+  // followed by RequestStart() invocation.
+  // If CONNECTING, ARC instance has not been booted properly, so do not
+  // restart it automatically.
+  if (session_started_ &&

[Luis Héctor Chávez, 2016/12/16 23:02:13]

In the same spirit as we did reset |reconnect_|, shouldn't you set
|session_started_ = false| here? Otherwise it's possible to call RequestStart()
if the state wasn't READY or STOPPING and have that be ignored. At the very
least it should be set to false in an else block.

Otherwise, please mention that the semantics of |session_started_| have changed
to mean something like "the user has requested for the ARC instance to be up".

[hidehiko, 2016/12/19 07:41:21]

IIUC, unlike |reconnect_|, |session_started_| should not be set to false here.

|reconnect_| is a flag representing if the current ARC instance gets READY state
already. On the other hand |session_started_| is a flag whether a client code
calls RequestStart/Stop().
So, also from the semantics point of view, |session_started_| should not be
changed here, and it keeps the original semantics.

For example, in the original code, "start -> ready -> crash -> (re-)start ->
ready -> crash -> (re-)start -> ..." is one of expected scenarios, which is not
reproducible if we reset |session_started_| here.

In this new code, |reconnect_| flag is corresponding to "state() == READY".
SetState(STOPPED) below is corresponding to |reconnect_ = false| conceptually.

I have left the current code and comment as is, because actually no semantics
change for |session_started_|. Of course welcome better way (like renaming var,
better comment), but I'd like to address it in the following CL, which focuses
on the state machine change. Actually the |session_started_| will be renamed to
|run_requested_| in the CL (thank you for suggestion!).

Maybe another way is change the condition as follows;

if (state() == State::READY ||
    (state() == State::STOPPING && session_started_)) {
  DCHECK(session_started_);
  :
}

The first condition (state == READY) is corresponding to |reconnect_ = true|
case.
The second condition (state == STOPPING && session_started_) is for
RequestStop() immediately followed by RequestStart() case.

Note that, if state == READY, session_started_ must be true always, because 1)
to be READY, RequestStart() must be called once, and 2) if even at least once
RequestStop() is called, the state is set to STOPPING.

WDYT?

[Luis Héctor Chávez, 2016/12/19 22:23:38]

I like the proposed condition much better, since it more closely aligns with the
new semantics.

Then again, I still think there should be *something* to notify the upper layers
that this won't be restarted anymore. Maybe a different |stop_reason|? In any
case, I'm fine with this particular item being resolved in a future change since
it's not a regression.

[hidehiko, 2016/12/20 13:38:02]

Sure, done. Just in case, could you take another look? With the change, we need
SetState(STOPPING) in Shutdown().
+1. I added TODO for the record. Maybe new stop_reason value, or alternatively
adding bool |retry_scheduled|? Anyway, let's discuss it separately.

+      (state() == State::READY || state() == State::STOPPING)) {
+    // There was a previous invocation and it crashed for some reason. Try
+    // starting ARC instance later again.
+    // Note that even |restart_delay_| is 0 (for testing), it needs to
+    // PostTask, because observer callback may call RequestStart()/Stop(),
+    // which can change restarting.
+    VLOG(0) << "ARC restarting";
+    restart_timer_.Start(FROM_HERE, restart_delay_,
+                         base::Bind(&ArcBridgeServiceImpl::StartArcSession,
+                                    weak_factory_.GetWeakPtr()));
+  }
+
   SetStopReason(stop_reason);
   SetState(State::STOPPED);
-
-  if (reconnect_) {
-    // There was a previous invocation and it crashed for some reason. Try
-    // starting the container again.
-    reconnect_ = false;
-    VLOG(0) << "ARC reconnecting";
-    if (use_delay_before_reconnecting_) {
-      // Instead of immediately trying to restart the container, give it some
-      // time to finish tearing down in case it is still in the process of
-      // stopping.
-      base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
-          FROM_HERE, base::Bind(&ArcBridgeServiceImpl::PrerequisitesChanged,
-                                weak_factory_.GetWeakPtr()),
-          base::TimeDelta::FromSeconds(kReconnectDelayInSeconds));
-    } else {
-      // Restart immediately.
-      PrerequisitesChanged();
-    }
-  }
 }
 
 }  // namespace arc