From 3715f7b2f774246e709fb2f94952b3bc3896f09c Mon Sep 17 00:00:00 2001 From: Changgyu Choi Date: Thu, 29 Apr 2021 10:47:31 +0900 Subject: [PATCH] Fix wrong response issues Closing fd should be performed on the thread. This patch modified what was handled on the main thread. Changes: * Remove FdErrorHandler() * Add to Disconnect() at socket destructor. * Add some logs for debugging. Change-Id: I436a6e987e5691970e0e76892b995e131d22e034 Signed-off-by: Changgyu Choi --- src/client/pkginfo_client.cc | 13 ------------- src/client/pkginfo_client.hh | 2 +- src/common/socket/abstract_socket.cc | 4 +++- src/common/socket/client_socket.cc | 2 -- src/common/socket/client_socket.hh | 1 - src/common/socket/data_socket.cc | 2 -- src/common/socket/data_socket.hh | 1 - src/common/socket/server_socket.cc | 2 -- src/common/socket/server_socket.hh | 2 -- src/server/pkg_request.cc | 2 +- src/server/runner.cc | 30 +++++++++++------------------- src/server/runner.hh | 2 -- src/server/worker_thread.cc | 15 ++++++++++----- 13 files changed, 26 insertions(+), 52 deletions(-) diff --git a/src/client/pkginfo_client.cc b/src/client/pkginfo_client.cc index 4cedf8c..8763c91 100644 --- a/src/client/pkginfo_client.cc +++ b/src/client/pkginfo_client.cc @@ -36,11 +36,6 @@ PkgInfoClient::PkgInfoClient( socket_ = std::make_unique(SOCK_PATH); } -PkgInfoClient::~PkgInfoClient() { - if (socket_ != nullptr) - socket_->Disconnect(); -} - bool PkgInfoClient::SendRequest() { if (socket_ == nullptr) { LOGE("Socket is not ready"); @@ -48,14 +43,12 @@ bool PkgInfoClient::SendRequest() { } if (!socket_->Connect()) { LOGE("Failed to connect client socket, try to direct access"); - socket_->Disconnect(); is_offline_ = true; return RequestHandlerDirectAccess(); } if (socket_->SendData(&req_type_, sizeof(req_type_)) != 0) { LOGE("fail to send data"); - socket_->Disconnect(); return false; } @@ -66,13 +59,11 @@ bool PkgInfoClient::SendRequest() { if (socket_->SendData(&len, sizeof(len)) != 0) { LOGE("fail to send data"); - socket_->Disconnect(); return false; } if (socket_->SendData(&raw[0], len) != 0) { LOGE("Fail to send data"); - socket_->Disconnect(); return false; } @@ -91,24 +82,20 @@ PkgInfoClient::GetResultParcel() { int len = 0; if (socket_->ReceiveData(&len, sizeof(len)) != 0 || len <= 0) { LOGE("Fail to receive data"); - socket_->Disconnect(); return nullptr; } unsigned char* raw = new (std::nothrow) unsigned char[len]; if (raw == nullptr) { LOGE("Out of memory"); - socket_->Disconnect(); return nullptr; } if (socket_->ReceiveData(raw, len) != 0) { LOGE("Fail to receive data"); - socket_->Disconnect(); delete[] raw; return nullptr; } - socket_->Disconnect(); auto res = pkgmgr_common::parcel::ParcelableFactory::GetInst().CreateParcel( raw, len); diff --git a/src/client/pkginfo_client.hh b/src/client/pkginfo_client.hh index eecef50..d874e8c 100644 --- a/src/client/pkginfo_client.hh +++ b/src/client/pkginfo_client.hh @@ -18,7 +18,7 @@ class PkgInfoClient { PkgInfoClient( std::shared_ptr parcel, uid_t uid, pkgmgr_common::ReqType req_type); - ~PkgInfoClient(); + ~PkgInfoClient() = default; bool SendRequest(); std::shared_ptr GetResultParcel(); diff --git a/src/common/socket/abstract_socket.cc b/src/common/socket/abstract_socket.cc index 765c80e..ad9d185 100644 --- a/src/common/socket/abstract_socket.cc +++ b/src/common/socket/abstract_socket.cc @@ -32,7 +32,9 @@ AbstractSocket::AbstractSocket(std::string path) AbstractSocket::AbstractSocket(int fd) : fd_(fd), addr_{} {} -AbstractSocket::~AbstractSocket() {} +AbstractSocket::~AbstractSocket() { + Disconnect(); +} int AbstractSocket::SendData(const void* buf, unsigned int size) { auto buffer = static_cast(buf); diff --git a/src/common/socket/client_socket.cc b/src/common/socket/client_socket.cc index 606e22e..c0cd8dc 100644 --- a/src/common/socket/client_socket.cc +++ b/src/common/socket/client_socket.cc @@ -30,8 +30,6 @@ namespace socket { ClientSocket::ClientSocket(std::string path) : AbstractSocket(std::move(path)) { } -ClientSocket::~ClientSocket() {} - void ClientSocket::SetTimeout(int timeout_msec) { if (timeout_msec == -1) timeout_msec = 5000; diff --git a/src/common/socket/client_socket.hh b/src/common/socket/client_socket.hh index bc08fea..6071de3 100644 --- a/src/common/socket/client_socket.hh +++ b/src/common/socket/client_socket.hh @@ -25,7 +25,6 @@ namespace socket { class EXPORT_API ClientSocket: public AbstractSocket { public: ClientSocket(std::string path); - ~ClientSocket(); bool Connect(); private: diff --git a/src/common/socket/data_socket.cc b/src/common/socket/data_socket.cc index 3dc9b5c..1799355 100644 --- a/src/common/socket/data_socket.cc +++ b/src/common/socket/data_socket.cc @@ -21,7 +21,5 @@ namespace socket { DataSocket::DataSocket(int fd) : AbstractSocket(fd) {} -DataSocket::~DataSocket() {} - } // namespace socket } // namespace pkgmgr_common diff --git a/src/common/socket/data_socket.hh b/src/common/socket/data_socket.hh index a4b2111..ea7303c 100644 --- a/src/common/socket/data_socket.hh +++ b/src/common/socket/data_socket.hh @@ -25,7 +25,6 @@ namespace socket { class EXPORT_API DataSocket: public AbstractSocket { public: DataSocket(int fd); - ~DataSocket(); }; } // namespace socket diff --git a/src/common/socket/server_socket.cc b/src/common/socket/server_socket.cc index aa13bcd..2a85a9e 100644 --- a/src/common/socket/server_socket.cc +++ b/src/common/socket/server_socket.cc @@ -66,8 +66,6 @@ ServerSocket::ServerSocket(std::string path) : AbstractSocket(std::move(path)) { LOGD("Server socket is created(%s)", path_.c_str()); } -ServerSocket::~ServerSocket() {} - int ServerSocket::Bind() { return bind(fd_, reinterpret_cast(&addr_), sizeof(addr_)); } diff --git a/src/common/socket/server_socket.hh b/src/common/socket/server_socket.hh index 1d58f9a..d9fab24 100644 --- a/src/common/socket/server_socket.hh +++ b/src/common/socket/server_socket.hh @@ -27,8 +27,6 @@ namespace socket { class EXPORT_API ServerSocket: public AbstractSocket { public: ServerSocket(std::string path); - ~ServerSocket(); - int Accept(); private: diff --git a/src/server/pkg_request.cc b/src/server/pkg_request.cc index d57aadf..0fc5f67 100644 --- a/src/server/pkg_request.cc +++ b/src/server/pkg_request.cc @@ -88,7 +88,7 @@ bool PkgRequest::SendData(unsigned char* data, int size) { if (socket_->SendData(&size, sizeof(size)) < 0) return false; - return (socket_->SendData(data, size) < 0); + return (socket_->SendData(data, size) == 0); } } // namespace pkgmgr_server diff --git a/src/server/runner.cc b/src/server/runner.cc index bb84747..866a91b 100644 --- a/src/server/runner.cc +++ b/src/server/runner.cc @@ -15,6 +15,7 @@ */ #include +#include #include #include @@ -34,13 +35,15 @@ namespace pkgmgr_server { static const std::string SOCK_PATH = "/run/pkgmgr-info-server"; -Runner::Runner(unsigned int thread_num) : sid_(-1) { +Runner::Runner(unsigned int thread_num) { /* thread_num_ <= hardware_concurrency */ thread_num_ = std::min(thread_num, std::thread::hardware_concurrency()); server_ = std::make_unique(SOCK_PATH); thread_pool_ = std::make_unique(thread_num_); - sid_ = g_unix_fd_add(server_->GetFd(), G_IO_IN, OnReceiveRequest, this); + auto condition = static_cast( + G_IO_IN | G_IO_ERR | G_IO_HUP | G_IO_NVAL); + sid_ = g_unix_fd_add(server_->GetFd(), condition, OnReceiveRequest, this); vconf_notify_key_changed(VCONFKEY_LANGSET, OnLanguageChange, this); std::unique_ptr locale( _get_system_locale(), std::free); @@ -56,31 +59,20 @@ Runner::~Runner() { vconf_ignore_key_changed(VCONFKEY_LANGSET, OnLanguageChange); } -int Runner::FdErrorHandler(int fd, GIOCondition cond, void* user_data) { - auto runner = static_cast(user_data); - if (static_cast(cond) & (G_IO_ERR | G_IO_HUP)) { - auto it = runner->sid_map_.find(fd); - if (it != runner->sid_map_.end()) { - g_source_remove(it->second); - runner->sid_map_.erase(it); - } - close(fd); +int Runner::OnReceiveRequest(int fd, GIOCondition cond, void* user_data) { + if (static_cast(cond) & (G_IO_ERR | G_IO_HUP | G_IO_NVAL)) { + LOGE("Invalid condition fd(%d) condition(%d)", fd, static_cast(cond)); + abort(); + return G_SOURCE_REMOVE; } - return G_SOURCE_CONTINUE; -} - -int Runner::OnReceiveRequest(int fd, GIOCondition cond, void* user_data) { auto runner = static_cast(user_data); int client_fd = runner->server_->Accept(); if (client_fd < 0) { - LOGE("Failed to Accept. (%d)", client_fd); + LOGE("Failed to Accept. errno(%d)", errno); return G_SOURCE_CONTINUE; } - auto condition = static_cast(G_IO_ERR | G_IO_HUP); - int sid = g_unix_fd_add(client_fd, condition, FdErrorHandler, runner); - runner->sid_map_[client_fd] = sid; runner->QueueRequest(client_fd); return G_SOURCE_CONTINUE; } diff --git a/src/server/runner.hh b/src/server/runner.hh index 702977f..678e89e 100644 --- a/src/server/runner.hh +++ b/src/server/runner.hh @@ -42,12 +42,10 @@ class EXPORT_API Runner { static int OnReceiveRequest(int fd, GIOCondition cond, void* user_data); static void OnLanguageChange(keynode_t* key, void* user_data); bool QueueRequest(int client_fd); - static int FdErrorHandler(int fd, GIOCondition cond, void* user_data); private: int sid_; unsigned int thread_num_; - std::unordered_map sid_map_; std::unique_ptr server_; std::unique_ptr thread_pool_; }; diff --git a/src/server/worker_thread.cc b/src/server/worker_thread.cc index bcd97d9..bd68f70 100644 --- a/src/server/worker_thread.cc +++ b/src/server/worker_thread.cc @@ -103,11 +103,12 @@ void WorkerThread::Run() { continue; } pkgmgr_common::ReqType type = req->GetRequestType(); - LOGD("Request type(%s), pid(%d)", + LOGW("Request type(%s), pid(%d)", pkgmgr_common::ReqTypeToString(type), req->GetSenderPID()); if (type <= pkgmgr_common::ReqType::REQ_TYPE_NONE || type >= pkgmgr_common::ReqType::MAX) { - LOGE("Request type is invalid (%d)", static_cast(type)); + LOGE("Request type is invalid (%d) pid(%d)", static_cast(type), + req->GetSenderPID()); pkgmgr_common::parcel::AbstractParcelable parcelable( 0, pkgmgr_common::parcel::ParcelableType::Unknown, PMINFO_R_ERROR); @@ -124,7 +125,7 @@ void WorkerThread::Run() { locale_.GetObject())) LOGE("Failed to handle request"); } catch (const std::exception& err) { - LOGE("Exception occurred (%s)", err.what()); + LOGE("Exception occurred (%s) pid(%d)", err.what(), req->GetSenderPID()); pkgmgr_common::parcel::AbstractParcelable parcelable( 0, pkgmgr_common::parcel::ParcelableType::Unknown, PMINFO_R_ERROR); tizen_base::Parcel p; @@ -133,7 +134,7 @@ void WorkerThread::Run() { req->SendData(&raw[0], raw.size()); continue; } catch (...) { - LOGE("Exception occurred"); + LOGE("Exception occurred pid(%d)", req->GetSenderPID()); pkgmgr_common::parcel::AbstractParcelable parcelable( 0, pkgmgr_common::parcel::ParcelableType::Unknown, PMINFO_R_ERROR); tizen_base::Parcel p; @@ -144,7 +145,11 @@ void WorkerThread::Run() { } std::vector result_data = handler[type]->ExtractResult(); - req->SendData(result_data.data(), result_data.size()); + if (req->SendData(result_data.data(), result_data.size()) == false) { + LOGE("Failed to send response pid(%d)", req->GetSenderPID()); + continue; + } + LOGW("Success response pid(%d)", req->GetSenderPID()); } } -- 2.7.4