Add unit tests for CLUSTER NODES parsing (#112)

- Includes a fix for a memory issue when the parsing fails. - Update the OOM test to include replica parsing. Signed-off-by: Björn Svensson <[email protected]>
valkey-io · Oct 14, 2024 · a97a74e · a97a74e
1 parent 0026c84
commit a97a74e
Show file tree

Hide file tree

Showing 5 changed files with 412 additions and 13 deletions.
diff --git a/Makefile b/Makefile
@@ -12,7 +12,7 @@ TEST_DIR = tests
 
 INCLUDE_DIR = include/valkey
 
-TEST_SRCS = $(TEST_DIR)/client_test.c
+TEST_SRCS = $(TEST_DIR)/client_test.c $(TEST_DIR)/ut_parse_cmd.c $(TEST_DIR)/ut_slotmap_update.c
 TEST_BINS = $(patsubst $(TEST_DIR)/%.c,$(TEST_DIR)/%,$(TEST_SRCS))
 
 SOURCES = $(filter-out $(wildcard $(SRC_DIR)/*tls.c) $(SRC_DIR)/rdma.c, $(wildcard $(SRC_DIR)/*.c))

diff --git a/src/cluster.c b/src/cluster.c
@@ -628,12 +628,12 @@ static int cluster_master_slave_mapping_with_name(valkeyClusterContext *cc,
             }
 
             if (node_old->slaves != NULL) {
-                node_old->slaves->free = NULL;
                 while (listLength(node_old->slaves) > 0) {
                     lnode = listFirst(node_old->slaves);
                     if (listAddNodeHead(node->slaves, lnode->value) == NULL) {
                         goto oom;
                     }
+                    node_old->slaves->free = NULL;
                     listDelNode(node_old->slaves, lnode);
                 }
                 listRelease(node_old->slaves);
@@ -952,7 +952,6 @@ static dict *parse_cluster_nodes(valkeyClusterContext *cc, valkeyReply *reply) {
                     ret = cluster_master_slave_mapping_with_name(
                         cc, &nodes_name, master, master->name);
                     if (ret != VALKEY_OK) {
-                        freeValkeyClusterNode(master);
                         goto error;
                     }
                 }
@@ -1042,8 +1041,19 @@ static dict *parse_cluster_nodes(valkeyClusterContext *cc, valkeyReply *reply) {
 error:
     sdsfreesplitres(part, count_part);
     sdsfreesplitres(slot_start_end, count_slot_start_end);
+    if (nodes_name != NULL) {
+        /* Only free parsed replicas since the `nodes` dict owns primary nodes. */
+        dictIterator di;
+        dictInitIterator(&di, nodes_name);
+        dictEntry *de;
+        while ((de = dictNext(&di))) {
+            valkeyClusterNode *node = dictGetEntryVal(de);
+            if (node->role == VALKEY_ROLE_SLAVE)
+                freeValkeyClusterNode(node);
+        }
+        dictRelease(nodes_name);
+    }
     dictRelease(nodes);
-    dictRelease(nodes_name);
     return NULL;
 }
 

diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt
@@ -85,6 +85,17 @@ if(ENABLE_RDMA)
   set_property(TEST client_test PROPERTY ENVIRONMENT "TEST_RDMA=1")
 endif()
 
+# Unit tests
+add_executable(ut_parse_cmd ut_parse_cmd.c test_utils.c)
+target_include_directories(ut_parse_cmd PRIVATE "${PROJECT_SOURCE_DIR}/src")
+target_link_libraries(ut_parse_cmd valkey)
+add_test(NAME ut_parse_cmd COMMAND "$<TARGET_FILE:ut_parse_cmd>")
+
+add_executable(ut_slotmap_update ut_slotmap_update.c)
+target_include_directories(ut_slotmap_update PRIVATE "${PROJECT_SOURCE_DIR}/src")
+target_link_libraries(ut_slotmap_update valkey)
+add_test(NAME ut_slotmap_update COMMAND "$<TARGET_FILE:ut_slotmap_update>")
+
 # Add cluster tests if we have libevent
 if (LIBEVENT_LIBRARY)
   add_executable(ct_async ct_async.c)
@@ -118,11 +129,6 @@ if (LIBEVENT_LIBRARY)
   target_link_libraries(ct_specific_nodes valkey ${TLS_LIBRARY} ${LIBEVENT_LIBRARY})
   add_test(NAME ct_specific_nodes COMMAND "$<TARGET_FILE:ct_specific_nodes>")
 
-  add_executable(ut_parse_cmd ut_parse_cmd.c test_utils.c)
-  target_include_directories(ut_parse_cmd PRIVATE "${PROJECT_SOURCE_DIR}/src")
-  target_link_libraries(ut_parse_cmd valkey ${TLS_LIBRARY})
-  add_test(NAME ut_parse_cmd COMMAND "$<TARGET_FILE:ut_parse_cmd>")
-
   if(LIBUV_LIBRARY)
     add_executable(ct_async_libuv ct_async_libuv.c)
     target_link_libraries(ct_async_libuv valkey ${TLS_LIBRARY} ${LIBUV_LIBRARY})

diff --git a/tests/ct_out_of_memory_handling.c b/tests/ct_out_of_memory_handling.c
@@ -125,6 +125,7 @@ void test_alloc_failure_handling(void) {
         cc = valkeyClusterContextInit();
         assert(cc);
     }
+    cc->flags |= VALKEYCLUSTER_FLAG_ADD_SLAVE;
 
     // Add nodes
     {
@@ -170,14 +171,14 @@ void test_alloc_failure_handling(void) {
 
     // Connect
     {
-        for (int i = 0; i < 128; ++i) {
+        for (int i = 0; i < 148; ++i) {
             prepare_allocation_test(cc, i);
             result = valkeyClusterConnect2(cc);
             assert(result == VALKEY_ERR);
             ASSERT_STR_EQ(cc->errstr, "Out of memory");
         }
 
-        prepare_allocation_test(cc, 128);
+        prepare_allocation_test(cc, 148);
         result = valkeyClusterConnect2(cc);
         assert(result == VALKEY_OK);
     }
@@ -493,6 +494,7 @@ void test_alloc_failure_handling_async(void) {
         acc = valkeyClusterAsyncContextInit();
         assert(acc);
     }
+    acc->cc->flags |= VALKEYCLUSTER_FLAG_ADD_SLAVE;
 
     // Set callbacks
     {
@@ -519,14 +521,14 @@ void test_alloc_failure_handling_async(void) {
 
     // Connect
     {
-        for (int i = 0; i < 126; ++i) {
+        for (int i = 0; i < 146; ++i) {
             prepare_allocation_test(acc->cc, i);
             result = valkeyClusterConnect2(acc->cc);
             assert(result == VALKEY_ERR);
             ASSERT_STR_EQ(acc->cc->errstr, "Out of memory");
         }
 
-        prepare_allocation_test(acc->cc, 126);
+        prepare_allocation_test(acc->cc, 146);
         result = valkeyClusterConnect2(acc->cc);
         assert(result == VALKEY_OK);
     }