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 4cedf8c..8763c91 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 eecef50..d874e8c 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 765c80e..ad9d185 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 606e22e..c0cd8dc 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 bc08fea..6071de3 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 3dc9b5c..1799355 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 a4b2111..ea7303c 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 aa13bcd..2a85a9e 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 1d58f9a..d9fab24 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 d57aadf..0fc5f67 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 bb84747..866a91b 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 702977f..678e89e 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 bcd97d9..bd68f70 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());
   }
 }