memceck race handling (#1152)

* removed time.sleep from tests teardown

* added async delete config

* fixed config and makefile

* moved memcheck to compiler flag

Author: DvirDukhan

.circleci/config.yml

@@ -94,7 +94,7 @@ jobs:
           command: |
               # Replace the default Redis server with one linked to libc malloc rather than jemalloc.
               git clone https://github.com/antirez/redis.git; cd redis; git checkout 6.0.1; make valgrind; make install; cd ..
-              make clean; make DEBUG=1 # Rebuild module at O0
+              make clean;
               make memcheck # Re-run the test suite, failing if definite memory leaks have been introduced.
           # Allow RediSearch global destructors.
           environment:

src/Makefile

@@ -200,5 +200,6 @@ endif
 endif
 	@$(MAKE) -C ../tests test
 
+memcheck: CFLAGS += -fno-omit-frame-pointer -g -ggdb -O0 -D MEMCHECK
 memcheck: redisgraph.so
 	@$(MAKE) -C ../tests memcheck

src/config.c

@@ -138,7 +138,7 @@ static int _Config_SetCacheSize(RedisModuleCtx *ctx, RedisModuleString *cache_si
 }
 
 // Initialize every module-level configuration to its default value.
-static void _Config_SetToDefaults(void) {
+static void _Config_SetToDefaults(RedisModuleCtx *ctx) {
 	// The thread pool's default size is equal to the system's number of cores.
 	int CPUCount = sysconf(_SC_NPROCESSORS_ONLN);
 	config.thread_count = (CPUCount != -1) ? CPUCount : 1;
@@ -154,14 +154,24 @@ static void _Config_SetToDefaults(void) {
 		config.vkey_entity_count = VKEY_ENTITY_COUNT_UNLIMITED;
 	}
 
+	// MEMCHECK compile flag;
+	#ifdef MEMCHECK
+		// Disable async delete during memcheck.
+		config.async_delete = false;
+		RedisModule_Log(ctx, "notice", "Graph deletion will be done synchronously.");
+	#else
+		// Always perform async delete when no checking for memory issues.
+		config.async_delete = true;
+		RedisModule_Log(ctx, "notice", "Graph deletion will be done asynchronously.");
+	#endif
 	// Always build transposed matrices by default.
 	config.maintain_transposed_matrices = true;
 	config.cache_size = CACHE_SIZE_DEFAULT;
 }
 
 int Config_Init(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) {
 	// Initialize the configuration to its default values.
-	_Config_SetToDefaults();
+	_Config_SetToDefaults(ctx);
 
 	if(argc % 2) {
 		// Emit an error if we received an odd number of arguments, as this indicates an invalid configuration.
@@ -222,3 +232,6 @@ uint64_t Config_GetCacheSize() {
 	return config.cache_size;
 }
 
+bool Config_GetAsyncDelete(void) {
+	return config.async_delete;
+}

src/config.h

@@ -12,6 +12,7 @@
 
 typedef struct {
 	int thread_count;                  // Thread count for thread pool.
+	bool async_delete;                 // If true, graph deletion is done asynchronously.
 	uint64_t cache_size;               // The cache size for each thread, per graph.
 	int omp_thread_count;              // Maximum number of OpenMP threads.
 	uint64_t vkey_entity_count;        // The limit of number of entities encoded at once for each RDB key.
@@ -37,3 +38,5 @@ bool Config_MaintainTranspose(void);
 // Return the cache size.
 uint64_t Config_GetCacheSize(void);
 
+// Return true if graph deletion is done asynchronously.
+bool Config_GetAsyncDelete(void);

src/graph/graphcontext.c

@@ -32,8 +32,15 @@ static inline void _GraphContext_IncreaseRefCount(GraphContext *gc) {
 
 static inline void _GraphContext_DecreaseRefCount(GraphContext *gc) {
 	// If the reference count is less than 0, the graph has been marked for deletion and no queries are active - free the graph.
-	if(__atomic_sub_fetch(&gc->ref_count, 1, __ATOMIC_RELAXED) < 0)
-		thpool_add_work(_thpool, _GraphContext_Free, gc);
+	if(__atomic_sub_fetch(&gc->ref_count, 1, __ATOMIC_RELAXED) < 0) {
+		if(Config_GetAsyncDelete()) {
+			// Async delete
+			thpool_add_work(_thpool, _GraphContext_Free, gc);
+		} else {
+			// Sync delete
+			_GraphContext_Free(gc);
+		}
+	}
 }
 
 //------------------------------------------------------------------------------

tests/flow/test_bulk_insertion.py

@@ -2,7 +2,6 @@
 import os
 import sys
 import csv
-import time
 import click
 from RLTest import Env
 from click.testing import CliRunner
@@ -28,9 +27,6 @@ def __init__(self):
         port = self.env.envRunner.port
         redis_graph = Graph("graph", redis_con)
 
-    def tearDown(self):
-        time.sleep(1)
-
     # Run bulk loader script and validate terminal output
     def test01_run_script(self):
         graphname = "graph"

tests/flow/test_imdb.py

@@ -1,6 +1,5 @@
 import os
 import sys
-import time
 from RLTest import Env
 from redisgraph import Graph
 from base import FlowTestsBase
@@ -32,7 +31,6 @@ def setUp(self):
 
     def tearDown(self):
         self.env.cmd('flushall')
-        time.sleep(1)
 
     def assert_reversed_pattern(self, query, resultset):
         # Test reversed pattern query.