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 <changyu.choi@samsung.com>

diff --git a/src/client/pkginfo_client.cc b/src/client/pkginfo_client.cc
index 4cedf8ca7c247edc2fc1b230e06c07ad45d3b18b..8763c9185b2ff16e66df7152edc40ee53ce29ef7 100644 (file)
--- a/src/client/pkginfo_client.cc
+++ b/src/client/pkginfo_client.cc
@@ -36,11 +36,6 @@ PkgInfoClient::PkgInfoClient(
   socket_ = std::make_unique<pkgmgr_common::socket::ClientSocket>(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 eecef50a5a0d68650ff9b29de112b72172ae29e9..d874e8c1190b7b6a036f8ca76bc15c1e637a11bb 100644 (file)
--- a/src/client/pkginfo_client.hh
+++ b/src/client/pkginfo_client.hh
@@ -18,7 +18,7 @@ class PkgInfoClient {
   PkgInfoClient(
       std::shared_ptr<pkgmgr_common::parcel::AbstractParcelable> parcel,
       uid_t uid, pkgmgr_common::ReqType req_type);
-  ~PkgInfoClient();
+  ~PkgInfoClient() = default;
   bool SendRequest();
   std::shared_ptr<pkgmgr_common::parcel::AbstractParcelable> GetResultParcel();
 

diff --git a/src/common/socket/abstract_socket.cc b/src/common/socket/abstract_socket.cc
index 765c80ef98b5126dce3bebb79274d654e0dbec05..ad9d185599279809fcbafd5cfb7c26e9da2b51b4 100644 (file)
--- 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<const unsigned char*>(buf);

diff --git a/src/common/socket/client_socket.cc b/src/common/socket/client_socket.cc
index 606e22e8a21065e72907565dfabf2368cdd333a0..c0cd8dc4ad4a908c073cabf90028522cef7a2602 100644 (file)
--- 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 bc08feabb7886eec56c769e2109fb648728cc2e3..6071de3ebdd75bc68e81ecd3feeb6709f4bb86ff 100644 (file)
--- 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 3dc9b5ce8c847e5e65f7f2c5838f47e1db1b95b9..17993555f216d0eb3e64c7d41101bb196ec50562 100644 (file)
--- 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 a4b211180fb8938e04aa58ceeb6705cdef8ac24b..ea7303c0e5fd6e009ad0a713af99f821608887b9 100644 (file)
--- 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 aa13bcd34e794814f626c20a2a77cfdd73b120b7..2a85a9ef3f39affb218e5ae85a42584624469b03 100644 (file)
--- 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<struct sockaddr*>(&addr_), sizeof(addr_));
 }

diff --git a/src/common/socket/server_socket.hh b/src/common/socket/server_socket.hh
index 1d58f9a0de481bdac63c3314c9901df2e242a8f2..d9fab241e58f31cfef8ae5b9de7dc89466a3ccf1 100644 (file)
--- 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 d57aadf35aa6607ef3b6b75ca8a4207b6f4e09d2..0fc5f67b6ce466639aa3b1044167669d4e46d8c0 100644 (file)
--- 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 bb847477d3a7c563341844b1c7fdf8ca6a12c1cb..866a91bd5dd8cdea497137d9e8c965091594b510 100644 (file)
--- a/src/server/runner.cc
+++ b/src/server/runner.cc
@@ -15,6 +15,7 @@
  */
 
 #include <glib-unix.h>
+#include <stdlib.h>
 #include <vconf.h>
 
 #include <algorithm>
@@ -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<pkgmgr_common::socket::ServerSocket>(SOCK_PATH);
   thread_pool_ = std::make_unique<WorkerThread>(thread_num_);
-  sid_ = g_unix_fd_add(server_->GetFd(), G_IO_IN, OnReceiveRequest, this);
+  auto condition = static_cast<GIOCondition>(
+      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<char, decltype(std::free)*> 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<Runner*>(user_data);
-  if (static_cast<int>(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<int>(cond) & (G_IO_ERR | G_IO_HUP | G_IO_NVAL)) {
+    LOGE("Invalid condition fd(%d) condition(%d)", fd, static_cast<int>(cond));
+    abort();
+    return G_SOURCE_REMOVE;
   }
 
-  return G_SOURCE_CONTINUE;
-}
-
-int Runner::OnReceiveRequest(int fd, GIOCondition cond, void* user_data) {
   auto runner = static_cast<Runner*>(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<GIOCondition>(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 702977f3b0aacd1e29947155333cd283de415833..678e89e54389635c8e8d736cd03e2ec8a9af884c 100644 (file)
--- 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<int, int> sid_map_;
   std::unique_ptr<pkgmgr_common::socket::ServerSocket> server_;
   std::unique_ptr<WorkerThread> thread_pool_;
 };

diff --git a/src/server/worker_thread.cc b/src/server/worker_thread.cc
index bcd97d93aca30e8c06892394a68c88b3fc983d94..bd68f702e798710be2d480e764a1db8d431666d6 100644 (file)
--- 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<int>(type));
+      LOGE("Request type is invalid (%d) pid(%d)", static_cast<int>(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<uint8_t> 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());
   }
 }