Fix race condition cold boot vs notifications (sonic-net#14)

oleksandrivantsiv · May 3, 2016 · c00b6ed · c00b6ed
1 parent 1f9baf4
commit c00b6ed
Show file tree

Hide file tree

Showing 4 changed files with 25 additions and 15 deletions.
diff --git a/common/redisclient.cpp b/common/redisclient.cpp
@@ -1,4 +1,5 @@
 #include "redisclient.h"
+#include "swss/logger.h"
 
 namespace swss
 {
@@ -190,7 +191,11 @@ std::shared_ptr<std::string> RedisClient::hget(std::string key, std::string fiel
     redisGetReply(m_db->getContext(), (void**)&reply);
 
     if (!reply)
-        throw std::runtime_error("HGET failed, memory exception");
+    {
+        SWSS_LOG_ERROR("HGET failed, null reply, memory exception: %s: %s", key.c_str(), field.c_str());
+
+        throw std::runtime_error("HGET failed, null reply, memory exception");
+    }
 
     if (reply->type == REDIS_REPLY_NIL)
     {
@@ -205,9 +210,11 @@ std::shared_ptr<std::string> RedisClient::hget(std::string key, std::string fiel
         return ptr;
     }
 
+    SWSS_LOG_ERROR("HGET failed, reply-type: %d, %s: %s", reply->type, key.c_str(), field.c_str());
+
     freeReplyObject(reply);
 
-    throw std::runtime_error("HGET failed, memory exception");
+    throw std::runtime_error("HGET failed, unexpected reply type, memory exception");
 }
 
 int64_t RedisClient::rpush(std::string list, std::string item)

diff --git a/syncd/syncd.cpp b/syncd/syncd.cpp
@@ -1212,4 +1212,8 @@ int main(int argc, char **argv)
     SWSS_LOG_NOTICE("calling api uninitialize");
 
     sai_api_uninitialize();
+
+    SWSS_LOG_NOTICE("uninitialize finished");
+
+    return EXIT_SUCCESS;
 }
diff --git a/syncd/syncd_notifications.cpp b/syncd/syncd_notifications.cpp
@@ -7,7 +7,7 @@ void send_notification(
 {
     SWSS_LOG_ENTER();
 
-    SWSS_LOG_DEBUG("sending notification: %s:%s", op.c_str(), data.c_str());
+    SWSS_LOG_NOTICE("%s %s", op.c_str(), data.c_str());
 
     notifications->send(op, data, entry);
 
@@ -39,7 +39,7 @@ void on_switch_state_change(
 }
 
 sai_fdb_entry_type_t getFdbEntryType(
-        _In_ uint32_t count, 
+        _In_ uint32_t count,
         _In_ const sai_attribute_t *list)
 {
     for (uint32_t idx = 0; idx < count; idx++)

diff --git a/syncd/syncd_reinit.cpp b/syncd/syncd_reinit.cpp
@@ -472,17 +472,6 @@ void helperCheckPortIds()
 {
     SWSS_LOG_ENTER();
 
-    // we need lock here since after initialization
-    // some notifications may appear and it may cause
-    // race condition for port id generation
-    std::lock_guard<std::mutex> lock(g_mutex);
-
-    // it may happen that after initialize we will receive
-    // some port notifications with port'ids that are not in
-    // redis db yet, so after checking VIDTORID map there will
-    // be entries and translate_vid_to_rid will generate new
-    // id's for ports
-
     auto laneMap = saiGetHardwareLaneMap();
 
     for (auto kv: laneMap)
@@ -542,6 +531,16 @@ void helperCheckVlanId()
 
 void onSyncdStart(bool warmStart)
 {
+    // it may happen that after initialize we will receive
+    // some port notifications with port'ids that are not in
+    // redis db yet, so after checking VIDTORID map there will
+    // be entries and translate_vid_to_rid will generate new
+    // id's for ports, this may cause race condition so we need
+    // to use a lock here to prevent that
+
+    std::lock_guard<std::mutex> lock(g_mutex);
+
+
     SWSS_LOG_ENTER();
 
     helperCheckLaneMap();