More graceful shutdown for ArcSession.

components/arc/arc_session.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_session.h"
 
 #include <fcntl.h>
 #include <grp.h>
 #include <poll.h>
 #include <unistd.h>
 
 #include <utility>
 
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
 #include "base/location.h"
 #include "base/macros.h"
 #include "base/memory/ptr_util.h"
 #include "base/posix/eintr_wrapper.h"
 #include "base/sys_info.h"
 #include "base/task_runner_util.h"
 #include "base/threading/thread_checker.h"
 #include "base/threading/thread_task_runner_handle.h"
-#include "base/threading/worker_pool.h"
 #include "chromeos/cryptohome/cryptohome_parameters.h"
 #include "chromeos/dbus/dbus_method_call_status.h"
 #include "chromeos/dbus/dbus_thread_manager.h"
 #include "chromeos/dbus/session_manager_client.h"
 #include "components/arc/arc_bridge_host_impl.h"
 #include "components/arc/arc_features.h"
 #include "components/user_manager/user_manager.h"
 #include "ipc/unix_domain_socket_util.h"
 #include "mojo/edk/embedder/embedder.h"
 #include "mojo/edk/embedder/platform_channel_pair.h"
@@ skipping 90 matching lines @@
   // At any state, Stop() can be called. It does not immediately stop the
   // instance, but will eventually stop it.
   // The actual stop will be notified via Observer::OnStopped().
   //
   // When Stop() is called, it makes various behavior based on the current
   // phase.
   //
   // NOT_STARTED:
   //   Do nothing. Immediately transition to the STOPPED state.
   // CREATING_SOCKET:
-  //   The main task of the phase runs on WorkerPool thread. So, Stop() just
+  //   The main task of the phase runs on BlockingPool thread. So, Stop() just
   //   sets the flag and return. On the main task completion, a callback
   //   will run on the main (practically UI) thread, and the flag is checked
   //   at the beginning of them. This should work under the assumption that
   //   the main tasks do not block indefinitely.
   // STARTING_INSTANCE:
   //   The ARC instance is starting via SessionManager. So, similar to
   //   CREATING_SOCKET case, Stop() just sets the flag and return. In its
   //   callback, it checks if ARC instance is successfully started or not.
   //   In case of success, a request to stop the ARC instance is sent to
   //   SessionManager. Its completion will be notified via ArcInstanceStopped.
   //   Otherwise, it just turns into STOPPED state.
   // CONNECTING_MOJO:
-  //   The main task runs on WorkerPool thread, but it is blocking call.
+  //   The main task runs on BlockingPool thread, but it is blocking call.
   //   So, Stop() sends a request to cancel the blocking by closing the pipe
   //   whose read side is also polled. Then, in its callback, similar to
   //   STARTING_INSTANCE, a request to stop the ARC instance is sent to
   //   SessionManager, and ArcInstanceStopped handles remaining procedure.
   // RUNNING:
   //   There is no more callback which runs on normal flow, so Stop() requests
   //   to stop the ARC instance via SessionManager.
   //
   // Another trigger to change the state coming from outside of this class
   // is an event ArcInstanceStopped() sent from SessionManager, when ARC
   // instace unexpectedly terminates. ArcInstanceStopped() turns the state into
   // STOPPED immediately.
   // This happens only when STARTING_INSTANCE, CONNECTING_MOJO or RUNNING
   // state.
   //
   // STARTING_INSTANCE:
   //   In OnInstanceStarted(), |state_| is checked at the beginning. If it is
   //   STOPPED, then ArcInstanceStopped() is called. Do nothing in that case.
   // CONNECTING_MOJO:
   //   Similar to Stop() case above, ArcInstanceStopped() also notifies to
-  //   WorkerPool() thread to cancel it to unblock the thread. In
+  //   BlockingPool thread to cancel it to unblock the thread. In
   //   OnMojoConnected(), similar to OnInstanceStarted(), check if |state_| is
   //   STOPPED, then do nothing.
   // RUNNING:
   //   It is not necessary to do anything special here.
   //
   // In NOT_STARTED or STOPPED state, the instance can be safely destructed.
   // Specifically, in STOPPED state, there may be inflight operations or
   // pending callbacks. Though, what they do is just do-nothing conceptually
   // and they can be safely ignored.
   //
@@ skipping 12 matching lines @@
     // The instance has started. Waiting for it to connect to the IPC bridge.
     CONNECTING_MOJO,
 
     // The instance is fully set up.
     RUNNING,
 
     // ARC is terminated.
     STOPPED,
   };
 
-  ArcSessionImpl();
+  explicit ArcSessionImpl(scoped_refptr<base::TaskRunner> blocking_task_runner);

[Luis Héctor Chávez, 2016/10/21 00:43:41]

nit: const scoped_refptr<...>&

[hidehiko, 2016/10/21 04:34:03]

Done.

   ~ArcSessionImpl() override;
 
   // ArcSession overrides:
   void Start() override;
   void Stop() override;
+  void OnShutdown() override;
 
  private:
   // Creates the UNIX socket on a worker pool and then processes its file
   // descriptor.
   static base::ScopedFD CreateSocket();
   void OnSocketCreated(base::ScopedFD fd);
 
   // DBus callback for StartArcInstance().
   void OnInstanceStarted(base::ScopedFD socket_fd,
                          StartArcInstanceResult result);
 
   // Synchronously accepts a connection on |socket_fd| and then processes the
   // connected socket's file descriptor.
   static base::ScopedFD ConnectMojo(base::ScopedFD socket_fd,
                                     base::ScopedFD cancel_fd);
   void OnMojoConnected(base::ScopedFD fd);
 
   // Request to stop ARC instance via DBus.
   void StopArcInstance();
 
   // chromeos::SessionManagerClient::Observer:
   void ArcInstanceStopped(bool clean) override;
 
   // Completes the termination procedure.
   void OnStopped(ArcBridgeService::StopReason reason);
 
+  // Checks whether a function runs on the thread where the instance is
+  // created.
+  base::ThreadChecker thread_checker_;
+
+  // Task runner to run a blocking tasks.
+  scoped_refptr<base::TaskRunner> blocking_task_runner_;
+
   // The state of the session.
   State state_ = State::NOT_STARTED;
 
   // When Stop() is called, this flag is set.
   bool stop_requested_ = false;
 
   // In CONNECTING_MOJO state, this is set to the write side of the pipe
   // to notify cancelling of the procedure.
   base::ScopedFD accept_cancel_pipe_;
 
   // Mojo endpoint.
   std::unique_ptr<mojom::ArcBridgeHost> arc_bridge_host_;
 
-  base::ThreadChecker thread_checker_;
-
   // WeakPtrFactory to use callbacks.
   base::WeakPtrFactory<ArcSessionImpl> weak_factory_;
 
  private:
   DISALLOW_COPY_AND_ASSIGN(ArcSessionImpl);
 };
 
-ArcSessionImpl::ArcSessionImpl() : weak_factory_(this) {
+ArcSessionImpl::ArcSessionImpl(
+    scoped_refptr<base::TaskRunner> blocking_task_runner)
+    : blocking_task_runner_(blocking_task_runner), weak_factory_(this) {
   chromeos::SessionManagerClient* client = GetSessionManagerClient();
   if (client == nullptr)
     return;
   client->AddObserver(this);
 }
 
 ArcSessionImpl::~ArcSessionImpl() {
   DCHECK(thread_checker_.CalledOnValidThread());
-  // TODO(hidehiko): CHECK if |state_| is in NOT_STARTED or STOPPED.
-  // Currently, specifically on shutdown, the state_ can be any value.
+  DCHECK(state_ == State::NOT_STARTED || state_ == State::STOPPED);
   chromeos::SessionManagerClient* client = GetSessionManagerClient();
   if (client == nullptr)
     return;
   client->RemoveObserver(this);
 }
 
 void ArcSessionImpl::Start() {
   DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK_EQ(state_, State::NOT_STARTED);
   VLOG(2) << "Starting ARC session.";
   VLOG(2) << "Creating socket...";
 
   state_ = State::CREATING_SOCKET;
   base::PostTaskAndReplyWithResult(
-      base::WorkerPool::GetTaskRunner(true).get(), FROM_HERE,
+      blocking_task_runner_.get(), FROM_HERE,
       base::Bind(&ArcSessionImpl::CreateSocket),
       base::Bind(&ArcSessionImpl::OnSocketCreated, weak_factory_.GetWeakPtr()));
 }
 
 // static
 base::ScopedFD ArcSessionImpl::CreateSocket() {
   base::FilePath socket_path(kArcBridgeSocketPath);
 
   int raw_fd = -1;
   if (!IPC::CreateServerUnixDomainSocket(socket_path, &raw_fd))
@@ skipping 98 matching lines @@
 
   // Prepare a pipe so that AcceptInstanceConnection can be interrupted on
   // Stop().
   base::ScopedFD cancel_fd;
   if (!CreatePipe(&cancel_fd, &accept_cancel_pipe_)) {
     OnStopped(ArcBridgeService::StopReason::GENERIC_BOOT_FAILURE);
     return;
   }
 
   base::PostTaskAndReplyWithResult(
-      base::WorkerPool::GetTaskRunner(true).get(), FROM_HERE,
+      blocking_task_runner_.get(), FROM_HERE,
       base::Bind(&ArcSessionImpl::ConnectMojo, base::Passed(&socket_fd),
                  base::Passed(&cancel_fd)),
       base::Bind(&ArcSessionImpl::OnMojoConnected, weak_factory_.GetWeakPtr()));
 }
 
 // static
 base::ScopedFD ArcSessionImpl::ConnectMojo(base::ScopedFD socket_fd,
                                            base::ScopedFD cancel_fd) {
   if (!WaitForSocketReadable(socket_fd.get(), cancel_fd.get())) {
     VLOG(1) << "Mojo connection was cancelled.";
@@ skipping 96 matching lines @@
       // On Chrome shutdown, it is not the case because the message loop is
       // already stopped here. Practically, it is not a problem because;
       // - On socket creating, it is ok to simply ignore such cases,
       // because we no-longer continue the bootstrap procedure.
       // - On starting instance, the container instance can be leaked.
       // Practically it is not problematic because the session manager will
       // clean it up.
       return;
 
     case State::CONNECTING_MOJO:
-      // Mojo connection is being waited on a WorkerPool thread.
+      // Mojo connection is being waited on a BlockingPool thread.
       // Request to cancel it. Following stopping procedure will run
       // in its callback.
-      DCHECK(accept_cancel_pipe_.get());

[hidehiko, 2016/10/20 10:09:08]

Note: I found that there is a case that this is true, when some error occurs and
CONNECTING_MOJO is stopping the instance (L463).

[Luis Héctor Chávez, 2016/10/21 00:43:41]

Acknowledged.

       accept_cancel_pipe_.reset();
       return;
 
     case State::RUNNING:
       // Now ARC instance is running. Request to stop it.
       StopArcInstance();
       return;
 
     case State::STOPPED:
       // The instance is already stopped. Do nothing.
@@ skipping 13 matching lines @@
   session_manager_client->StopArcInstance(
       base::Bind(&DoNothingInstanceStopped));
 }
 
 void ArcSessionImpl::ArcInstanceStopped(bool clean) {
   DCHECK(thread_checker_.CalledOnValidThread());
   VLOG(1) << "Notified that ARC instance is stopped "
           << (clean ? "cleanly" : "uncleanly");
 
   // In case that crash happens during before the Mojo channel is connected,
-  // unlock the WorkerPool thread.
+  // unlock the BlockingPool thread.
   accept_cancel_pipe_.reset();
 
   ArcBridgeService::StopReason reason;
   if (stop_requested_) {
     // If the ARC instance is stopped after its explicit request,
     // return SHUTDOWN.
     reason = ArcBridgeService::StopReason::SHUTDOWN;
   } else if (clean) {
     // If the ARC instance is stopped, but it is not explicitly requested,
     // then this is triggered by some failure during the starting procedure.
@@ skipping 10 matching lines @@
   DCHECK(thread_checker_.CalledOnValidThread());
   // OnStopped() should be called once per instance.
   DCHECK_NE(state_, State::STOPPED);
   VLOG(2) << "ARC session is stopped.";
   arc_bridge_host_.reset();
   state_ = State::STOPPED;
   for (auto& observer : observer_list_)
     observer.OnStopped(reason);
 }
 
+void ArcSessionImpl::OnShutdown() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  stop_requested_ = true;
+  if (state_ == State::STOPPED)
+    return;
+
+  // Here, the message loop is already stopped, and the Chrome will be soon
+  // shutdown. Thus, it is not necessary to take care about restarting case.
+  // If ArcSession is waiting for mojo connection, cancels it. The BlockingPool
+  // will be joined later.
+  accept_cancel_pipe_.reset();
+
+  // After Chrome's shutdown, SessionManager shuts down the ARC instance,

[Luis Héctor Chávez, 2016/10/21 00:43:41]

I'd still want this code to be robust by itself and not depend on SessionManager
automatically cleaning stuff up. Can you still call StopArcInstance() if the
state indicates that the container is running? But please conserve the comment
below that the UI thread is gone so OnStopped must be explicitly called.

[hidehiko, 2016/10/21 04:34:03]

Could you tell me what more robustness do you want to ensure here?
Or, do you think we cannot rely on SessionManager's behavior?

CC'ed hashimoto@, the DBus expert. He may give us more advice.

[Luis Héctor Chávez, 2016/10/21 06:34:20]

We can rely on SessionManager's _current_ behavior (since both the explicit
shutting down of the container and the browser exiting code paths have the same
container-terminating code), but I'd rather be explicit if we can, in case we
need to change its behavior. e.g. at some point there were plans of giving
Android a bit more time to shutdown, so that code might diverge slightly in the
future.

The only thing we cannot rely on is having the callback dispatched due to the
lack of a message loop, but the outgoing call should be able to get through
without any issues.

[hashimoto, 2016/10/21 07:20:27]

IIUC unclean shutdown is also covered by SessionManagerService::HandleExit.
(https://chromium.googlesource.com/chromiumos/platform2/+/master/login_manager...)
I think SessionManager must continue shutting down the container whenever Chrome
dies, because Chrome can die in ways other than clean shutdown (e.g. crash, or
getting killed by SessionManager), but the container must be killed before
Chrome gets restarted.

Also, SessionManagerService::HandleExit() gives the container 3 seconds to
shutdown, while SessionManagerImpl::StopArcInstance() only gives it 1 second.
Although I don't think this difference was introduced intentionally, IIUC
calling StopArcInstance during Chrome shutdown results in unclean container
shutdown happening more frequently.

[Luis Héctor Chávez, 2016/10/21 07:53:43]

Oh that was unintentional :/ we should swap the timeouts in session_manager, so
that the clean exit is 3s and the non-clean shutdown is just 1.

+  // so we do not need to worry about it here.
+  // Directly set to the STOPPED stateby OnStopped(). Note that callsing

[Luis Héctor Chávez, 2016/10/21 00:43:41]

nit: s/callsing/calling/

[hidehiko, 2016/10/21 04:34:03]

Done.

+  // StopArcInstance() may not work well. At least, because the UI thread is
+  // already stopped here, ArcInstanceStopped() callback cannot be invoked.
+  OnStopped(ArcBridgeService::StopReason::SHUTDOWN);
+}
+
 }  // namespace
 
 ArcSession::ArcSession() = default;
 ArcSession::~ArcSession() = default;
 
 void ArcSession::AddObserver(Observer* observer) {
   observer_list_.AddObserver(observer);
 }
 
 void ArcSession::RemoveObserver(Observer* observer) {
   observer_list_.RemoveObserver(observer);
 }
 
 // static
-std::unique_ptr<ArcSession> ArcSession::Create() {
-  return base::MakeUnique<ArcSessionImpl>();
+std::unique_ptr<ArcSession> ArcSession::Create(
+    scoped_refptr<base::TaskRunner> blocking_task_runner) {
+  return base::MakeUnique<ArcSessionImpl>(blocking_task_runner);
 }
 
 }  // namespace arc