shill: fix synchronous shutdown
When the system has a cellular modem, shill takes
care to wait for the cellular connection to be torn
down, before exiting. To do so, shill waits for some
asynchronous termination actions to complete.
In order to wait for these termination actions,
ChromeosDBusDaemon does the following:
1. Call ChromeosDaemon::Quit(), with a completion callback
which will break out of the message loop.
2. Let ChromeosDaemon::Quit() initiate any appropriate
asynchronous calls.
3. Run the message loop.
4. Expect ChromeosDaemon to invoke the callback from 1,
to break out the message loop.
This scheme works fine when we do have asynchronous
work. But we run into problems when we do _not_ have
asynchronous work. The problem is that, in this case,
ChromeosDaemon::Quit() invokes the completion callback
synchronously.
Because the completion callback executes synchronously,
we attempt to break out of the message loop before the
message loop is actually running. When the message loop
does start running, it keeps running indefinitely.
That's because the completion callback has already run.
To fix this, we have ChromeosDaemon::Quit() indicate
whether or not termination can proceed synchronously.
If termination can proceed synchronously,
ChromeosDBusDaemon::OnShutdown() does not attempt
to run the message loop.
While there, we modify ChromeosDaemon to _not_
invoke the completion callback, if all work completed
synchronously. This part of the change avoids the
following log message:
Message loop not running, ignoring BreakLoop().
Arguably, this second change isn't necessary. The log
message is a VERBOSE1, which is normally off. Nonetheless,
I think skipping the callback in the synchronous case makes
the logic easier to understand.
Bug: None
BUG=chrome-os-partner:45309
TEST=unit tests, manual
Manual test
-----------
- On a device _without_ a cellular modem, run
date && restart shill > /dev/null && date
The differences between the two timestamps should be small
(approx. 1 second).
- On a device _with_ a cellular modem, run
(ff_debug --level -1 && ff_debug +daemon) > /dev/null \
&& date && restart shill > /dev/null && date \
&& egrep 'Will wait' /var/log/net.log
You should see 'Will wait for termination actions to complete'
Change-Id: Ieeb7a66a29a49f7c0b133044462a89bb1dee1ffc
diff --git a/chromeos_daemon_unittest.cc b/chromeos_daemon_unittest.cc
index 0e9fd0c..ca01cb9 100644
--- a/chromeos_daemon_unittest.cc
+++ b/chromeos_daemon_unittest.cc
@@ -67,12 +67,18 @@
}
virtual ~ChromeosDaemonForTest() {}
+ bool quit_result() { return quit_result_; }
+
void RunMessageLoop() override { dispatcher_->DispatchForever(); }
- void Quit(const base::Closure& completion_callback) override {
- ChromeosDaemon::Quit(completion_callback);
+ bool Quit(const base::Closure& completion_callback) override {
+ quit_result_ = ChromeosDaemon::Quit(completion_callback);
dispatcher_->PostTask(base::MessageLoop::QuitClosure());
+ return quit_result_;
}
+
+ private:
+ bool quit_result_;
};
class ChromeosDaemonTest : public Test {
@@ -176,7 +182,7 @@
manager->TerminationActionComplete(name);
}
-TEST_F(ChromeosDaemonTest, Quit) {
+TEST_F(ChromeosDaemonTest, QuitWithTerminationAction) {
// This expectation verifies that the termination actions are invoked.
EXPECT_CALL(*this, TerminationAction())
.WillOnce(CompleteAction(manager_, "daemon test"));
@@ -188,12 +194,20 @@
// Run Daemon::Quit() after the daemon starts running.
dispatcher_.PostTask(
- Bind(&ChromeosDaemon::Quit,
+ Bind(IgnoreResult(&ChromeosDaemon::Quit),
Unretained(&daemon_),
Bind(&ChromeosDaemonTest::TerminationCompleted,
Unretained(this))));
RunDaemon();
+ EXPECT_FALSE(daemon_.quit_result());
+}
+
+TEST_F(ChromeosDaemonTest, QuitWithoutTerminationActions) {
+ EXPECT_CALL(*this, TerminationCompleted()).Times(0);
+ EXPECT_TRUE(daemon_.Quit(
+ Bind(&ChromeosDaemonTest::TerminationCompleted,
+ Unretained(this))));
}
TEST_F(ChromeosDaemonTest, ApplySettings) {