[shill] Switch from home-grown EventQueue to Chrome's MessageLoop
Many other chrome os daemons have borrowed chrome's message loop abstraction,
which supports posting Task and DelayedTask objects to the loop for later
processing. There are also Callback* objects in there that we can use for
other things, as I've done with the GlibIOInputHandler. Unit tests contain
some trivial examples.
More documentation in:
base/message_loop.h
base/task.h
base/callback.h
BUG=chromium-os:15105
TEST=build, unit tests
Change-Id: Ia53dcbdf70da45e4ceda7eed1debbc4bb507446e
Reviewed-on: http://gerrit.chromium.org/gerrit/713
Tested-by: Chris Masone <cmasone@chromium.org>
Reviewed-by: Paul Stewart <pstew@chromium.org>
diff --git a/Makefile b/Makefile
index 72d72a5..580c6be 100644
--- a/Makefile
+++ b/Makefile
@@ -9,14 +9,17 @@
PKG_CONFIG ?= pkg-config
DBUSXX_XML2CPP = dbusxx-xml2cpp
-
-BASE_LIBS = -lbase -lchromeos -lpthread -lrt
+# libevent, gdk and gtk-2.0 are needed to leverage chrome's MessageLoop
+# TODO(cmasone): explore if newer versions of libbase let us avoid this.
+BASE_LIBS = -lbase -lchromeos -levent -lpthread -lrt
BASE_INCLUDE_DIRS = -I..
BASE_LIB_DIRS =
LIBS = $(BASE_LIBS)
-INCLUDE_DIRS = $(BASE_INCLUDE_DIRS) $(shell $(PKG_CONFIG) --cflags glib-2.0)
-LIB_DIRS = $(BASE_LIB_DIRS) $(shell $(PKG_CONFIG) --libs glib-2.0)
+INCLUDE_DIRS = $(BASE_INCLUDE_DIRS) $(shell $(PKG_CONFIG) --cflags glib-2.0 \
+ gdk-2.0 gtk+-2.0)
+LIB_DIRS = $(BASE_LIB_DIRS) $(shell $(PKG_CONFIG) --libs glib-2.0 \
+ gdk-2.0 gtk+-2.0)
TEST_LIBS = $(BASE_LIBS) -lgmock -lgtest
TEST_INCLUDE_DIRS = $(INCLUDE_DIRS)
diff --git a/device.cc b/device.cc
index d199746..6037ac8 100644
--- a/device.cc
+++ b/device.cc
@@ -8,15 +8,17 @@
#include <string>
#include <base/logging.h>
+#include <base/ref_counted.h>
#include "shill/control_interface.h"
#include "shill/device.h"
+#include "shill/shill_event.h"
using std::string;
namespace shill {
Device::Device(ControlInterface *control_interface,
- EventDispatcher */* dispatcher */)
+ EventDispatcher *dispatcher)
: adaptor_(control_interface->CreateDeviceAdaptor(this)),
running_(false) {
// Initialize Interface monitor, so we can detect new interfaces
diff --git a/device.h b/device.h
index 53e2608..20ae0e4 100644
--- a/device.h
+++ b/device.h
@@ -18,11 +18,12 @@
// A constructor for the Device object
explicit Device(ControlInterface *control_interface,
EventDispatcher *dispatcher);
+
void Start();
void Stop();
protected:
- ~Device();
+ virtual ~Device();
private:
DeviceAdaptorInterface *adaptor_;
diff --git a/device_info.cc b/device_info.cc
index d86c66f..584e36d 100644
--- a/device_info.cc
+++ b/device_info.cc
@@ -15,7 +15,10 @@
#include <linux/rtnetlink.h>
#include <string>
+#include <base/callback.h>
#include <base/logging.h>
+#include <base/hash_tables.h>
+#include <base/scoped_ptr.h>
#include "shill/control_interface.h"
#include "shill/service.h"
@@ -27,7 +30,7 @@
DeviceInfo::DeviceInfo(EventDispatcher *dispatcher)
: running_(false),
dispatcher_(dispatcher),
- rtnl_callback_(this, &DeviceInfo::ParseRTNL),
+ rtnl_callback_(NewCallback(this, &DeviceInfo::ParseRTNL)),
rtnl_socket_(-1),
request_flags_(0),
request_sequence_(0) {
@@ -63,7 +66,7 @@
}
rtnl_handler_.reset(dispatcher_->CreateInputHandler(rtnl_socket_,
- &rtnl_callback_));
+ rtnl_callback_.get()));
running_ = true;
request_flags_ = REQUEST_LINK | REQUEST_ADDR | REQUEST_ROUTE;
diff --git a/device_info.h b/device_info.h
index 5b6b18d..d6f20af 100644
--- a/device_info.h
+++ b/device_info.h
@@ -5,6 +5,7 @@
#ifndef SHILL_DEVICE_INFO_
#define SHILL_DEVICE_INFO_
+#include <base/callback.h>
#include <base/hash_tables.h>
#include <base/scoped_ptr.h>
@@ -28,7 +29,7 @@
bool running_;
EventDispatcher *dispatcher_;
- ClassCallback<DeviceInfo, InputData *> rtnl_callback_;
+ scoped_ptr<Callback1<InputData *>::Type> rtnl_callback_;
int rtnl_socket_;
base::hash_map<int, Device *> devices_;
scoped_ptr<IOInputHandler> rtnl_handler_;
diff --git a/shill_event.cc b/shill_event.cc
index b78680c..e49ad55 100644
--- a/shill_event.cc
+++ b/shill_event.cc
@@ -5,64 +5,25 @@
#include <stdio.h>
#include <glib.h>
+#include <base/callback.h>
+#include <base/message_loop_proxy.h>
+
#include "shill/shill_event.h"
namespace shill {
-EventQueueItem::EventQueueItem(EventDispatcher *dispatcher)
- : dispatcher_(dispatcher) {
- dispatcher->RegisterCallbackQueue(this);
-}
-
-EventQueueItem::~EventQueueItem() {
- dispatcher_->UnregisterCallbackQueue(this);
-}
-
-void EventQueueItem::AlertDispatcher() {
- dispatcher_->ExecuteOnIdle();
-}
-
-void EventDispatcher::DispatchEvents() {
- for (size_t idx = 0; idx < queue_list_.size(); ++idx)
- queue_list_[idx]->Dispatch();
-}
-
-static gboolean DispatchEventsHandler(gpointer data) {
- EventDispatcher *dispatcher = static_cast<EventDispatcher *>(data);
- dispatcher->DispatchEvents();
- return false;
-}
-
-void EventDispatcher::ExecuteOnIdle() {
- g_idle_add(&DispatchEventsHandler, this);
-}
-
-void EventDispatcher::RegisterCallbackQueue(EventQueueItem *queue) {
- queue_list_.push_back(queue);
-}
-
-void EventDispatcher::UnregisterCallbackQueue(EventQueueItem *queue) {
- for (size_t idx = 0; idx < queue_list_.size(); ++idx) {
- if (queue_list_[idx] == queue) {
- queue_list_.erase(queue_list_.begin() + idx);
- return;
- }
- }
-}
-
-static gboolean DispatchIOHandler(GIOChannel *chan, GIOCondition cond,
+static gboolean DispatchIOHandler(GIOChannel *chan,
+ GIOCondition cond,
gpointer data);
class GlibIOInputHandler : public IOInputHandler {
public:
GIOChannel *channel_;
- Callback<InputData *> *callback_;
+ Callback1<InputData *>::Type *callback_;
guint source_id_;
- EventDispatcher *dispatcher_;
- GlibIOInputHandler(EventDispatcher *dispatcher, int fd,
- Callback<InputData *> *callback) :
- dispatcher_(dispatcher), callback_(callback) {
+ GlibIOInputHandler(int fd, Callback1<InputData*>::Type *callback)
+ : callback_(callback) {
channel_ = g_io_channel_unix_new(fd);
g_io_channel_set_close_on_unref(channel_, TRUE);
source_id_ = g_io_add_watch(channel_,
@@ -78,7 +39,8 @@
}
};
-static gboolean DispatchIOHandler(GIOChannel *chan, GIOCondition cond,
+static gboolean DispatchIOHandler(GIOChannel *chan,
+ GIOCondition cond,
gpointer data) {
GlibIOInputHandler *handler = static_cast<GlibIOInputHandler *>(data);
unsigned char buf[4096];
@@ -100,10 +62,29 @@
return TRUE;
}
+EventDispatcher::EventDispatcher()
+ : dont_use_directly_(new MessageLoopForUI),
+ message_loop_proxy_(base::MessageLoopProxy::CreateForCurrentThread()) {
+}
-IOInputHandler *EventDispatcher::CreateInputHandler(int fd,
- Callback<InputData *> *callback) {
- return new GlibIOInputHandler(this, fd, callback);
+EventDispatcher::~EventDispatcher() {}
+
+void EventDispatcher::DispatchForever() {
+ MessageLoop::current()->Run();
+}
+
+bool EventDispatcher::PostTask(Task* task) {
+ message_loop_proxy_->PostTask(FROM_HERE, task);
+}
+
+bool EventDispatcher::PostDelayedTask(Task* task, int64 delay_ms) {
+ message_loop_proxy_->PostDelayedTask(FROM_HERE, task, delay_ms);
+}
+
+IOInputHandler *EventDispatcher::CreateInputHandler(
+ int fd,
+ Callback1<InputData*>::Type *callback) {
+ return new GlibIOInputHandler(fd, callback);
}
} // namespace shill
diff --git a/shill_event.h b/shill_event.h
index cda8872..9ae4304 100644
--- a/shill_event.h
+++ b/shill_event.h
@@ -7,96 +7,19 @@
#include <vector>
+#include <base/basictypes.h>
+#include <base/callback.h>
+#include <base/message_loop.h>
+#include <base/ref_counted.h>
+#include <base/scoped_ptr.h>
+
+namespace base {
+class MessageLoopProxy;
+} // namespace base
+class Task;
+
namespace shill {
-// This is a pure-virtual base class for callback objects which can be
-// queued up and called later. The callback virtual method takes a single
-// argument, which will be handed to the dispatcher to call on all listeners.
-template <typename Arg>
-class Callback {
- public:
- virtual ~Callback() {}
- virtual void Run(Arg arg) = 0;
-};
-
-// This is a callback subclass that contains an object and method to call.
-// These methods take a passed-in argument specific to the callback type.
-template <typename Class, typename Arg>
-class ClassCallback : public Callback<Arg> {
- public:
- typedef void (Class::*MethodType)(Arg arg);
-
- ClassCallback(Class* object, MethodType method)
- : object_(object), method_(method) {}
- ~ClassCallback() {}
-
- void Run(Arg arg) {
- (object_->*method_)(arg);
- }
-
- private:
- Class* object_;
- MethodType method_;
-};
-
-// This is the event queue superclass, which contains a function for
-// dispatching all events in the queue to their respective listeners.
-// A common "AlertDispatcher()" function is used by subclasses to alert
-// the central dispatcher that events have been queued and a dispatch
-// should be performed soon.
-class EventDispatcher;
-class EventQueueItem {
- public:
- EventQueueItem(EventDispatcher *dispatcher);
- ~EventQueueItem();
- virtual void Dispatch() = 0;
- void AlertDispatcher();
- private:
- EventDispatcher *dispatcher_;
-};
-
-// This is a template subclass of EventQueueItem which is specific to
-// a particular argument type. This object contains a queue of events
-// waiting for delivery to liesteners, and a list of even callbacks --
-// the listeners for this event.
-template <typename Arg>
-class EventQueue : public EventQueueItem {
- typedef Callback<Arg>CallbackType;
-
- public:
- explicit EventQueue(EventDispatcher *dispatcher)
- : EventQueueItem(dispatcher) {}
-
- inline void AddCallback(CallbackType *cb) {
- callback_list_.push_back(cb);
- }
-
- void RemoveCallback(CallbackType *cb) {
- for (size_t event_idx = 0; event_idx < callback_list_.size(); ++event_idx) {
- if (callback_list_[event_idx] == cb) {
- callback_list_.erase(callback_list_.begin() + event_idx);
- return;
- }
- }
- }
-
- void Dispatch() {
- for (size_t event_idx = 0; event_idx < event_queue_.size(); ++event_idx)
- for (size_t call_idx = 0; call_idx < callback_list_.size(); ++call_idx)
- callback_list_[call_idx]->Run(event_queue_[event_idx]);
- event_queue_.clear();
- }
-
- void AddEvent(Arg arg) {
- event_queue_.push_back(arg);
- AlertDispatcher();
- }
-
- private:
- std::vector<CallbackType *> callback_list_;
- std::vector<Arg> event_queue_;
-};
-
struct InputData {
unsigned char *buf;
size_t len;
@@ -113,13 +36,20 @@
// their listeners during the idle loop.
class EventDispatcher {
public:
- void DispatchEvents();
- void ExecuteOnIdle();
- void RegisterCallbackQueue(EventQueueItem *queue);
- void UnregisterCallbackQueue(EventQueueItem *queue);
- IOInputHandler *CreateInputHandler(int fd, Callback<InputData *> *callback);
+ EventDispatcher();
+ virtual ~EventDispatcher();
+ void DispatchForever();
+
+ // These are thin wrappers around calls of the same name in
+ // <base/message_loop_proxy.h>
+ bool PostTask(Task* task);
+ bool PostDelayedTask(Task* task, int64 delay_ms);
+
+ IOInputHandler *CreateInputHandler(int fd,
+ Callback1<InputData*>::Type *callback);
private:
- std::vector<EventQueueItem*> queue_list_;
+ scoped_ptr<MessageLoop> dont_use_directly_;
+ scoped_refptr<base::MessageLoopProxy> message_loop_proxy_;
};
} // namespace shill
diff --git a/shill_main.cc b/shill_main.cc
index b446f7a..ea45859 100644
--- a/shill_main.cc
+++ b/shill_main.cc
@@ -5,6 +5,7 @@
#include <time.h>
#include <string>
+#include <base/at_exit.h>
#include <base/command_line.h>
#include <base/file_path.h>
#include <base/logging.h>
@@ -50,6 +51,7 @@
int main(int argc, char** argv) {
+ base::AtExitManager exit_manager;
CommandLine::Init(argc, argv);
CommandLine* cl = CommandLine::ForCurrentProcess();
diff --git a/shill_unittest.cc b/shill_unittest.cc
index fce22a6..e448d11 100644
--- a/shill_unittest.cc
+++ b/shill_unittest.cc
@@ -6,6 +6,8 @@
#include <glib.h>
#include <base/logging.h>
+#include <base/message_loop_proxy.h>
+#include <base/stringprintf.h>
#include <gtest/gtest.h>
#include <gmock/gmock.h>
@@ -15,71 +17,77 @@
namespace shill {
using ::testing::Test;
using ::testing::_;
-using ::testing::DoAll;
-using ::testing::InSequence;
+using ::testing::Gt;
using ::testing::NotNull;
using ::testing::Return;
-using ::testing::SetArgumentPointee;
using ::testing::StrictMock;
class MockEventDispatchTester {
public:
explicit MockEventDispatchTester(EventDispatcher *dispatcher)
- : dispatcher_(dispatcher),
- triggered_(false),
- int_callback_(new ClassCallback<MockEventDispatchTester, int>(this,
- &MockEventDispatchTester::HandleInt)),
- int_callback_queue_(dispatcher),
- got_data_(false),
- data_callback_(new ClassCallback<MockEventDispatchTester, InputData *>
- (this, &MockEventDispatchTester::HandleData)) {
- int_callback_queue_.AddCallback(int_callback_);
- timer_source_ = g_timeout_add_seconds(1,
- &MockEventDispatchTester::CallbackFunc, this);
+ : dispatcher_(dispatcher),
+ triggered_(false),
+ callback_count_(0),
+ got_data_(false),
+ data_callback_(NULL),
+ input_handler_(NULL),
+ tester_factory_(this) {
}
- ~MockEventDispatchTester() {
- RemoveCallback();
- delete(int_callback_);
- g_source_remove(timer_source_);
+ void ScheduleTimedTasks() {
+ dispatcher_->PostDelayedTask(
+ tester_factory_.NewRunnableMethod(&MockEventDispatchTester::Trigger),
+ 10);
+ // also set up a failsafe, so the test still exits even if something goes
+ // wrong. The Factory owns the RunnableMethod, but we get a pointer to it.
+ failsafe_ = tester_factory_.NewRunnableMethod(
+ &MockEventDispatchTester::QuitRegardless);
+ dispatcher_->PostDelayedTask(failsafe_, 100);
}
- void TimerFunction(int counter) {
- printf("Callback func called #%d\n", counter);
- if (counter > 1)
- int_callback_queue_.AddEvent(counter);
- }
-
- void HandleInt(int arg) {
- printf("MockEventDispatchTester handling int %d\n", arg);
- // Depending on the course of events, we may be called either once or
- // twice depending on timing. Only call the mock routine once.
+ void RescheduleUnlessTriggered() {
+ ++callback_count_;
if (!triggered_) {
- CallbackComplete(arg);
- triggered_ = true;
+ dispatcher_->PostTask(
+ tester_factory_.NewRunnableMethod(
+ &MockEventDispatchTester::RescheduleUnlessTriggered));
+ } else {
+ failsafe_->Cancel();
+ QuitRegardless();
}
}
- bool GetTrigger() { return triggered_; }
- void ResetTrigger() { triggered_ = false; }
- void RemoveCallback() { int_callback_queue_.RemoveCallback(int_callback_); }
+ void QuitRegardless() {
+ dispatcher_->PostTask(new MessageLoop::QuitTask);
+ }
+ void Trigger() {
+ LOG(INFO) << "MockEventDispatchTester handling " << callback_count_;
+ CallbackComplete(callback_count_);
+ triggered_ = true;
+ }
void HandleData(InputData *inputData) {
- printf("MockEventDispatchTester handling data len %d %.*s\n",
- inputData->len, inputData->len, inputData->buf);
+ LOG(INFO) << "MockEventDispatchTester handling data len "
+ << base::StringPrintf("%d %.*s", inputData->len,
+ inputData->len, inputData->buf);
got_data_ = true;
IOComplete(inputData->len);
+ QuitRegardless();
}
+
bool GetData() { return got_data_; }
void ListenIO(int fd) {
- input_handler_ = dispatcher_->CreateInputHandler(fd, data_callback_);
+ data_callback_.reset(NewCallback(this,
+ &MockEventDispatchTester::HandleData));
+ input_handler_.reset(dispatcher_->CreateInputHandler(fd,
+ data_callback_.get()));
}
void StopListenIO() {
got_data_ = false;
- delete(input_handler_);
+ input_handler_.reset(NULL);
}
MOCK_METHOD1(CallbackComplete, void(int));
@@ -87,19 +95,12 @@
private:
EventDispatcher *dispatcher_;
bool triggered_;
- Callback<int> *int_callback_;
- EventQueue<int>int_callback_queue_;
+ int callback_count_;
bool got_data_;
- Callback<InputData *> *data_callback_;
- IOInputHandler *input_handler_;
- int timer_source_;
- static gboolean CallbackFunc(gpointer data) {
- static int i = 0;
- MockEventDispatchTester *dispatcher_test =
- static_cast<MockEventDispatchTester *>(data);
- dispatcher_test->TimerFunction(i++);
- return true;
- }
+ scoped_ptr<Callback1<InputData*>::Type> data_callback_;
+ scoped_ptr<IOInputHandler> input_handler_;
+ ScopedRunnableMethodFactory<MockEventDispatchTester> tester_factory_;
+ CancelableTask* failsafe_;
};
class ShillDaemonTest : public Test {
@@ -108,11 +109,14 @@
: daemon_(&config_, new DBusControl()),
dispatcher_(&daemon_.dispatcher_),
dispatcher_test_(dispatcher_),
- device_info_(dispatcher_) {}
+ device_info_(dispatcher_),
+ factory_(this) {
+ }
+ virtual ~ShillDaemonTest() {}
virtual void SetUp() {
// Tests initialization done by the daemon's constructor
- EXPECT_NE((Config *)NULL, daemon_.config_);
- EXPECT_NE((ControlInterface *)NULL, daemon_.control_);
+ ASSERT_NE(reinterpret_cast<Config*>(NULL), daemon_.config_);
+ ASSERT_NE(reinterpret_cast<ControlInterface*>(NULL), daemon_.control_);
}
int DeviceInfoFlags() { return device_info_.request_flags_; }
protected:
@@ -121,41 +125,29 @@
DeviceInfo device_info_;
EventDispatcher *dispatcher_;
StrictMock<MockEventDispatchTester> dispatcher_test_;
+ ScopedRunnableMethodFactory<ShillDaemonTest> factory_;
};
TEST_F(ShillDaemonTest, EventDispatcher) {
- EXPECT_CALL(dispatcher_test_, CallbackComplete(2));
-
- // Crank the glib main loop a few times until the event handler triggers
- for (int main_loop_count = 0;
- main_loop_count < 6 && !dispatcher_test_.GetTrigger(); ++main_loop_count)
- g_main_context_iteration(NULL, TRUE);
-
- EXPECT_EQ(dispatcher_test_.GetTrigger(), true);
-
- dispatcher_test_.ResetTrigger();
- dispatcher_test_.RemoveCallback();
-
- // Crank the glib main loop a few more times, ensuring there is no trigger
- for (int main_loop_count = 0;
- main_loop_count < 6 && !dispatcher_test_.GetTrigger(); ++main_loop_count)
- g_main_context_iteration(NULL, TRUE);
-
+ EXPECT_CALL(dispatcher_test_, CallbackComplete(Gt(0)));
+ dispatcher_test_.ScheduleTimedTasks();
+ dispatcher_test_.RescheduleUnlessTriggered();
+ dispatcher_->DispatchForever();
EXPECT_CALL(dispatcher_test_, IOComplete(16));
int pipefd[2];
- EXPECT_EQ(pipe(pipefd), 0);
+ ASSERT_EQ(pipe(pipefd), 0);
dispatcher_test_.ListenIO(pipefd[0]);
- EXPECT_EQ(write(pipefd[1], "This is a test?!", 16), 16);
+ ASSERT_EQ(write(pipefd[1], "This is a test?!", 16), 16);
- // Crank the glib main loop a few times
- for (int main_loop_count = 0;
- main_loop_count < 6 && !dispatcher_test_.GetData(); ++main_loop_count)
- g_main_context_iteration(NULL, TRUE);
-
+ dispatcher_->DispatchForever();
dispatcher_test_.StopListenIO();
+}
+
+TEST_F(ShillDaemonTest, DeviceEnumeration) {
+ // TODO(cmasone): Make this unit test use message loop primitives.
// Start our own private device_info and make sure request flags clear
device_info_.Start();
diff --git a/testrunner.cc b/testrunner.cc
index b3bb2c0..f31439e 100644
--- a/testrunner.cc
+++ b/testrunner.cc
@@ -2,9 +2,11 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
+#include <base/at_exit.h>
#include <gtest/gtest.h>
int main(int argc, char** argv) {
+ base::AtExitManager exit_manager;
::testing::InitGoogleTest(&argc, argv);
return RUN_ALL_TESTS();
}