Remove signal handler and improve PubSub::listem_message()

common/Makefile.am

@@ -45,7 +45,6 @@ libswsscommon_la_SOURCES = \
     select.cpp                \
     selectableevent.cpp       \
     selectabletimer.cpp       \
-    signalhandlerhelper.cpp   \
     consumertable.cpp         \
     consumertablebase.cpp     \
     consumerstatetable.cpp    \

common/configdb.cpp

@@ -33,22 +33,24 @@ void ConfigDBConnector_Native::db_connect(string db_name, bool wait_for_init, bo
             for (;;)
             {
                 auto item = pubsub->listen_message();
-                if (item["type"] == "pmessage")
+                if (item.empty() || item["type"] != "pmessage")
                 {
-                    string channel = item["channel"];
-                    size_t pos = channel.find(':');
-                    string key;
-                    if (pos != string::npos)
-                    {
-                        key = channel.substr(pos + 1);
-                    }
-                    if (key == INIT_INDICATOR)
+                    continue;
+                }
+
+                string channel = item["channel"];
+                size_t pos = channel.find(':');
+                string key;
+                if (pos != string::npos)
+                {
+                    key = channel.substr(pos + 1);
+                }
+                if (key == INIT_INDICATOR)
+                {
+                    initialized = client.get(INIT_INDICATOR);
+                    if (initialized && !initialized->empty())
                     {
-                        initialized = client.get(INIT_INDICATOR);
-                        if (initialized && !initialized->empty())
-                        {
-                            break;
-                        }
+                        break;
                     }
                 }
             }

common/configdb.h

@@ -94,10 +94,10 @@ class ConfigDBConnector_Native : public SonicV2Connector_Native
             if init_data_handler:
                 init_data_handler(init_callback_data)
 
-            while not SignalHandlerHelper.checkSignal(SIGNAL_INT):
+            while True:
                 item = self.pubsub.listen_message()
                 if 'type' not in item:
-                    # When timeout or cancelled, item will not contains 'type' 
+                    # When timeout or cancelled or break by signal, item will not contains 'type' 
                     continue
 
                 if item['type'] == 'pmessage':

common/pubsub.cpp

@@ -101,7 +101,7 @@ MessageResultPair PubSub::get_message_internal(double timeout)
             throw RedisError("Failed to select", m_subscribe->getContext());
 
         case Select::TIMEOUT:
-        case Select::SIGNALINT:
+        case Select::SIGNAL:
             return ret;
 
         case Select::OBJECT:
@@ -131,17 +131,7 @@ MessageResultPair PubSub::get_message_internal(double timeout)
 std::map<std::string, std::string> PubSub::listen_message()
 {
     const double GET_MESSAGE_INTERVAL = 600.0; // in seconds
-    MessageResultPair ret;
-    for (;;)
-    {
-        ret = get_message_internal(GET_MESSAGE_INTERVAL);
-        if (!ret.second.empty() || ret.first == Select::SIGNALINT)
-        {
-            break;
-        }
-    }
-
-    return ret.second;
+    return get_message_internal(GET_MESSAGE_INTERVAL).second;
 }
 
 shared_ptr<RedisReply> PubSub::popEventBuffer()

common/select.cpp

@@ -1,7 +1,6 @@
 #include "common/selectable.h"
 #include "common/logger.h"
 #include "common/select.h"
-#include "common/signalhandlerhelper.h"
 #include <algorithm>
 #include <stdio.h>
 #include <sys/time.h>
@@ -95,16 +94,11 @@ int Select::poll_descriptors(Selectable **c, unsigned int timeout)
     std::vector<struct epoll_event> events(sz_selectables);
     int ret;
 
-    do
+    ret = ::epoll_wait(m_epoll_fd, events.data(), sz_selectables, timeout);
+    if (ret == -1 && errno == EINTR)
     {
-        ret = ::epoll_wait(m_epoll_fd, events.data(), sz_selectables, timeout);
-    }
-    while(ret == -1 && errno == EINTR && !SignalHandlerHelper::checkSignal(Signals::SIGNAL_INT)); // Retry the select if the process was interrupted by a signal
-
-    if (SignalHandlerHelper::checkSignal(Signals::SIGNAL_INT))
-    {
-        // Return if the epoll_wait was interrupted by SIGTERM
-        return Select::SIGNALINT;
+        // Return if the epoll_wait was interrupted by signal
+        return Select::SIGNAL;
     }
 
     if (ret < 0)
@@ -175,7 +169,6 @@ int Select::select(Selectable **c, int timeout)
     ret = poll_descriptors(c, timeout);
 
     return ret;
-
 }
 
 bool Select::isQueueEmpty()
@@ -198,8 +191,8 @@ std::string Select::resultToString(int result)
         case swss::Select::TIMEOUT:
             return "TIMEOUT";
 
-        case swss::Select::SIGNALINT:
-            return "SIGNALINT";
+        case swss::Select::SIGNAL:
+            return "SIGNAL";
 
         default:
             SWSS_LOG_WARN("unknown select result: %d", result);

common/select.h

@@ -30,7 +30,7 @@ class Select
         OBJECT = 0,
         ERROR = 1,
         TIMEOUT = 2,
-        SIGNALINT = 3,// Read operation interrupted by SIGINT
+        SIGNAL = 3,// Read operation interrupted by SIGNAL
     };
 
     int select(Selectable **c, int timeout = -1);

pyext/swsscommon.i

@@ -19,7 +19,6 @@
 #include "pubsub.h"
 #include "select.h"
 #include "selectable.h"
-#include "signalhandlerhelper.h"
 #include "rediscommand.h"
 #include "table.h"
 #include "countertable.h"
@@ -156,7 +155,6 @@ T castSelectableObj(swss::Selectable *temp)
 %include "pubsub.h"
 %include "selectable.h"
 %include "select.h"
-%include "signalhandlerhelper.h"
 %include "rediscommand.h"
 %include "redispipeline.h"
 %include "redisselect.h"

tests/conftest.py

@@ -7,5 +7,6 @@
 
 @pytest.fixture(scope="session", autouse=True)
 def prepare(request):
-    SonicDBConfig.initialize(existing_file)
+    if not SonicDBConfig.isInit():
+        SonicDBConfig.initialize(existing_file)
 

tests/test_signalhandler_ut.py

@@ -1,33 +1,59 @@
 import signal
 import os
 import pytest
+import time
+import threading
+
 from swsscommon import swsscommon
-from swsscommon.swsscommon import SignalHandlerHelper
+from swsscommon.swsscommon import SonicV2Connector, SonicDBConfig
 
-def dummy_signal_handler(signum, stack):
-    # ignore signal so UT will not break
-    pass
+CurrentSignalNumber = 0
+StopThread = False
 
-def test_SignalHandler():
-    signal.signal(signal.SIGUSR1, dummy_signal_handler)
+def python_signal_handler(signum, stack):
+    global CurrentSignalNumber
+    CurrentSignalNumber = signum
 
-    # Register SIGUSER1
-    SignalHandlerHelper.registerSignalHandler(signal.SIGUSR1)
-    happened = SignalHandlerHelper.checkSignal(signal.SIGUSR1)
-    assert happened == False
+def pubsub_thread():
+    global StopThread
+    connector =swsscommon.ConfigDBConnector()
+    connector.db_connect('CONFIG_DB')
+    connector.subscribe('A', lambda a: None)
 
-    # trigger SIGUSER manually
-    os.kill(os.getpid(), signal.SIGUSR1)
-    happened = SignalHandlerHelper.checkSignal(signal.SIGUSR1)
-    assert happened == True
-
-    # Reset signal
-    SignalHandlerHelper.resetSignal(signal.SIGUSR1)
-    happened = SignalHandlerHelper.checkSignal(signal.SIGUSR1)
-    assert happened == False
+    # listen_message method should not break Python signal handler
+    pubsub = connector.get_redis_client(connector.db_name).pubsub()
+    pubsub.psubscribe("__keyspace@{}__:*".format(connector.get_dbid(connector.db_name)))
+    while not StopThread:
+        pubsub.listen_message()
+
+def check_signal_can_break_pubsub(signalId):
+    global CurrentSignalNumber
+    CurrentSignalNumber = 0
+    global StopThread
+    StopThread = False
+    signal.signal(signalId, python_signal_handler)
+
+    test_thread = threading.Thread(target=pubsub_thread)
+    test_thread.start()
+
+    # check thread is running
+    time.sleep(2)
+    assert test_thread.is_alive() == True
+
+    os.kill(os.getpid(), signalId)
+
+    # signal will not break listen() method
+    time.sleep(2)
+    assert test_thread.is_alive() == True
+
+    # python signal handler will receive signal
+    assert CurrentSignalNumber == signalId
 
-    # un-register signal handler
-    SignalHandlerHelper.restoreSignalHandler(signal.SIGUSR1)
-    os.kill(os.getpid(), signal.SIGUSR1)
-    happened = SignalHandlerHelper.checkSignal(signal.SIGUSR1)
-    assert happened == False
+    # stop pubsub thread
+    StopThread = True
+    time.sleep(2)
+    assert test_thread.is_alive() == False
+
+def test_SignalIntAndSigTerm():
+    check_signal_can_break_pubsub(signal.SIGINT)
+    check_signal_can_break_pubsub(signal.SIGTERM)