From 59e07bdd30b6a0378c8fb793ab3c1f19f7edb386 Mon Sep 17 00:00:00 2001 From: Kyungwook Tak Date: Tue, 7 Jun 2016 15:27:44 +0900 Subject: [PATCH] Check & Clean all TODOs. Socket::create added to divided domain socket creation and client socket connection easily. Mutex locks in mainloop and service are not needed because callback add/erase are done by single thread (main thread) only. Change-Id: I56a18cb81218908f79e130186c11ec206955240c Signed-off-by: Kyungwook Tak --- src/framework/client/handle-ext.cpp | 1 - src/framework/common/dispatcher.cpp | 3 ++- src/framework/common/dispatcher.h | 8 ++++---- src/framework/common/mainloop.cpp | 3 --- src/framework/common/service.cpp | 30 +++++++++++------------------- src/framework/common/socket.cpp | 12 ++++++++++++ src/framework/common/socket.h | 14 ++++++++------ 7 files changed, 37 insertions(+), 34 deletions(-) diff --git a/src/framework/client/handle-ext.cpp b/src/framework/client/handle-ext.cpp index 3a7b065..9d1aea1 100644 --- a/src/framework/client/handle-ext.cpp +++ b/src/framework/client/handle-ext.cpp @@ -79,7 +79,6 @@ void HandleExt::dispatchAsync(const std::shared_ptr &f) this->m_isRunning = true; this->m_stop = false; - // TODO: how to handle exceptions in workers this->m_worker = std::thread([this, f] { DEBUG("client async thread dispatched! tid: " << std::this_thread::get_id()); diff --git a/src/framework/common/dispatcher.cpp b/src/framework/common/dispatcher.cpp index 019edf7..ea15f9c 100644 --- a/src/framework/common/dispatcher.cpp +++ b/src/framework/common/dispatcher.cpp @@ -33,7 +33,8 @@ Dispatcher::Dispatcher(SockId sockId) noexcept : m_sockId(sockId) void Dispatcher::connect() { - this->m_connection = std::make_shared(Socket::connect(this->m_sockId)); + this->m_connection = std::make_shared( + Socket::create(this->m_sockId, Socket::Type::CLIENT)); } bool Dispatcher::isConnected() const noexcept diff --git a/src/framework/common/dispatcher.h b/src/framework/common/dispatcher.h index 716e4e0..211d3e2 100644 --- a/src/framework/common/dispatcher.h +++ b/src/framework/common/dispatcher.h @@ -53,13 +53,13 @@ private: template Type Dispatcher::methodCall(Args &&...args) { - if (!isConnected()) - connect(); + if (!this->isConnected()) + this->connect(); - m_connection->send(BinaryQueue::Serialize(std::forward(args)...).pop()); + this->m_connection->send(BinaryQueue::Serialize(std::forward(args)...).pop()); BinaryQueue q; - q.push(m_connection->receive()); + q.push(this->m_connection->receive()); Type response; q.Deserialize(response); diff --git a/src/framework/common/mainloop.cpp b/src/framework/common/mainloop.cpp index 02875af..3316c0a 100644 --- a/src/framework/common/mainloop.cpp +++ b/src/framework/common/mainloop.cpp @@ -61,7 +61,6 @@ void Mainloop::run(int timeout) void Mainloop::addEventSource(int fd, uint32_t event, Callback &&callback) { - /* TODO: use scoped-lock to thread safe on member variables */ if (this->m_callbacks.count(fd) != 0) ThrowExc(CSR_ERROR_SERVER, "event source on fd[" << fd << "] already added!"); @@ -82,7 +81,6 @@ void Mainloop::addEventSource(int fd, uint32_t event, Callback &&callback) void Mainloop::removeEventSource(int fd) { - /* TODO: use scoped-lock to thread safe on member variables */ if (this->m_callbacks.count(fd) == 0) ThrowExc(CSR_ERROR_SERVER, "event source on fd[" << fd << "] isn't added at all"); @@ -134,7 +132,6 @@ void Mainloop::dispatch(int timeout) } for (int i = 0; i < nfds; i++) { - /* TODO: use scoped-lock to thread safe on member variables */ int fd = event[i].data.fd; if (this->m_callbacks.count(fd) == 0) diff --git a/src/framework/common/service.cpp b/src/framework/common/service.cpp index c95234f..de5399e 100644 --- a/src/framework/common/service.cpp +++ b/src/framework/common/service.cpp @@ -54,7 +54,7 @@ void Service::start(int timeout) INFO("Service start!"); for (const auto &id : this->m_sockIds) { - auto socket = std::make_shared(id); + auto socket = std::make_shared(Socket::create(id, Socket::Type::SERVER)); DEBUG("Get systemd socket[" << socket->getFd() << "] for sock id: " << static_cast(id)); @@ -71,10 +71,9 @@ void Service::start(int timeout) this->m_loop.run(timeout); } -void Service::setNewConnectionCallback(const ConnCallback &/*callback*/) +void Service::setNewConnectionCallback(const ConnCallback &callback) { - /* TODO: scoped-lock */ - this->m_onNewConnection = [&](const ConnShPtr & connection) { + this->m_onNewConnection = [this, &callback](const ConnShPtr &connection) { if (!connection) ThrowExc(CSR_ERROR_SERVER, "onNewConnection called but ConnShPtr is nullptr."); @@ -82,14 +81,11 @@ void Service::setNewConnectionCallback(const ConnCallback &/*callback*/) INFO("welcome! accepted client socket fd[" << fd << "]"); - /* - // TODO: disable temporarily - if (callback) - callback(connection); - */ + if (callback) + callback(connection); this->m_loop.addEventSource(fd, EPOLLIN | EPOLLHUP | EPOLLRDHUP, - [ &, fd](uint32_t event) { + [&, fd](uint32_t event) { DEBUG("read event comes in to fd[" << fd << "]"); if (this->m_connectionRegistry.count(fd) == 0) @@ -113,10 +109,9 @@ void Service::setNewConnectionCallback(const ConnCallback &/*callback*/) }; } -void Service::setCloseConnectionCallback(const ConnCallback &/*callback*/) +void Service::setCloseConnectionCallback(const ConnCallback &callback) { - /* TODO: scoped-lock */ - this->m_onCloseConnection = [&](const ConnShPtr & connection) { + this->m_onCloseConnection = [this, &callback](const ConnShPtr &connection) { if (!connection) ThrowExc(CSR_ERROR_SERVER, "no connection to close"); @@ -129,13 +124,10 @@ void Service::setCloseConnectionCallback(const ConnCallback &/*callback*/) INFO("good-bye! close socket fd[" << fd << "]"); this->m_loop.removeEventSource(fd); - this->m_connectionRegistry.erase(fd); /* scoped-lock needed? */ + this->m_connectionRegistry.erase(fd); - /* - // TODO: disable temporarily - if (callback) - callback(connection); - */ + if (callback) + callback(connection); }; } diff --git a/src/framework/common/socket.cpp b/src/framework/common/socket.cpp index b582797..d60e1e2 100644 --- a/src/framework/common/socket.cpp +++ b/src/framework/common/socket.cpp @@ -57,6 +57,18 @@ int createSystemdSocket(const std::string &path) } // namespace anonymous +Socket Socket::create(SockId sockId, Socket::Type type) +{ + switch (type) { + case Socket::Type::SERVER: + return Socket(sockId); + case Socket::Type::CLIENT: + return Socket::connect(sockId); + default: + ThrowExc(CSR_ERROR_SOCKET, "Invalid type to Socket::create"); + } +} + Socket::Socket(SockId sockId, int fd) : m_sockId(sockId), m_fd(fd) { if (this->m_fd < 0) diff --git a/src/framework/common/socket.h b/src/framework/common/socket.h index a896307..c902902 100644 --- a/src/framework/common/socket.h +++ b/src/framework/common/socket.h @@ -31,11 +31,12 @@ namespace Csr { class API Socket { public: - // Socket with accepted / connected - Socket(SockId sockId, int fd); + enum class Type : int { + SERVER = 0x01, + CLIENT = 0x02 + }; - // Create systemd socket - Socket(SockId sockId); + static Socket create(SockId, Socket::Type); Socket(const Socket &) = delete; Socket &operator=(const Socket &) = delete; @@ -53,10 +54,11 @@ public: RawBuffer read(void) const; void write(const RawBuffer &data) const; - /* TODO: can it be constructor? */ +private: static Socket connect(SockId); + Socket(SockId sockId, int fd); + Socket(SockId sockId); -private: SockId m_sockId; int m_fd; }; -- 2.7.4