[Tarantool-patches] [PATCH 2/2] func: fix not unloading of unused modules

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

C functions are loaded from .so/.dylib dynamic libraries. A
library is loaded when any function from there is called first
time. And was supposed to be unloaded, when all its functions are
dropped from the schema (box.schema.func.drop()), and none of them
is still in a call. But the unloading part was broken.

In fact, box.schema.func.drop() never unloaded anything. Moreover,
when functions from the module were added again without a restart,
it led to a second mmap of the same module. And so on, the same
library could be loaded any number of times.

The problem was in a useless flag in struct module preventing its
unloading even when it is totally unused. It is dropped.

Closes #4648
---
 src/box/func.c                             |  16 +--
 src/box/func.h                             |   2 -
 src/lib/core/errinj.h                      |   1 +
 test/box/function1.c                       |  15 +++
 test/box/gh-4648-func-load-unload.result   | 137 +++++++++++++++++++++
 test/box/gh-4648-func-load-unload.test.lua |  63 ++++++++++
 test/box/suite.ini                         |   2 +-
 7 files changed, 226 insertions(+), 10 deletions(-)
 create mode 100644 test/box/gh-4648-func-load-unload.result
 create mode 100644 test/box/gh-4648-func-load-unload.test.lua

diff --git a/src/box/func.c b/src/box/func.c
index d9481b714..431577127 100644
--- a/src/box/func.c
+++ b/src/box/func.c
@@ -35,6 +35,7 @@
 #include "lua/utils.h"
 #include "lua/call.h"
 #include "error.h"
+#include "errinj.h"
 #include "diag.h"
 #include "port.h"
 #include "schema.h"
@@ -260,7 +261,6 @@ module_load(const char *package, const char *package_end)
 	module->package[package_len] = 0;
 	rlist_create(&module->funcs);
 	module->calls = 0;
-	module->is_unloading = false;
 	char dir_name[] = "/tmp/tntXXXXXX";
 	if (mkdtemp(dir_name) == NULL) {
 		diag_set(SystemError, "failed to create unique dir name");
@@ -283,7 +283,9 @@ module_load(const char *package, const char *package_end)
 			  package, dlerror());
 		goto error;
 	}
-
+	struct errinj *e = errinj(ERRINJ_DYN_MODULE_COUNT, ERRINJ_INT);
+	if (e != NULL)
+		++e->iparam;
 	return module;
 error:
 	free(module);
@@ -293,6 +295,9 @@ error:
 static void
 module_delete(struct module *module)
 {
+	struct errinj *e = errinj(ERRINJ_DYN_MODULE_COUNT, ERRINJ_INT);
+	if (e != NULL)
+		--e->iparam;
 	dlclose(module->handle);
 	TRASH(module);
 	free(module);
@@ -304,10 +309,8 @@ module_delete(struct module *module)
 static void
 module_gc(struct module *module)
 {
-	if (!module->is_unloading || !rlist_empty(&module->funcs) ||
-	     module->calls != 0)
-		return;
-	module_delete(module);
+	if (rlist_empty(&module->funcs) && module->calls == 0)
+		module_delete(module);
 }
 
 /*
@@ -351,7 +354,6 @@ module_reload(const char *package, const char *package_end, struct module **modu
 	module_cache_del(package, package_end);
 	if (module_cache_put(new_module) != 0)
 		goto restore;
-	old_module->is_unloading = true;
 	module_gc(old_module);
 	*module = new_module;
 	return 0;
diff --git a/src/box/func.h b/src/box/func.h
index 18b83faac..581e468cb 100644
--- a/src/box/func.h
+++ b/src/box/func.h
@@ -54,8 +54,6 @@ struct module {
 	struct rlist funcs;
 	/** Count of active calls. */
 	size_t calls;
-	/** True if module is being unloaded. */
-	bool is_unloading;
 	/** Module's package name. */
 	char package[0];
 };
diff --git a/src/lib/core/errinj.h b/src/lib/core/errinj.h
index 3072a00ea..672da2119 100644
--- a/src/lib/core/errinj.h
+++ b/src/lib/core/errinj.h
@@ -134,6 +134,7 @@ struct errinj {
 	_(ERRINJ_SQL_NAME_NORMALIZATION, ERRINJ_BOOL, {.bparam = false}) \
 	_(ERRINJ_COIO_SENDFILE_CHUNK, ERRINJ_INT, {.iparam = -1}) \
 	_(ERRINJ_SWIM_FD_ONLY, ERRINJ_BOOL, {.bparam = false}) \
+	_(ERRINJ_DYN_MODULE_COUNT, ERRINJ_INT, {.iparam = 0}) \
 
 ENUM0(errinj_id, ERRINJ_LIST);
 extern struct errinj errinjs[];
diff --git a/test/box/function1.c b/test/box/function1.c
index b0d983e2b..fd434c280 100644
--- a/test/box/function1.c
+++ b/test/box/function1.c
@@ -229,3 +229,18 @@ test_yield(box_function_ctx_t *ctx, const char *args, const char *args_end)
 	printf("ok - yield\n");
 	return 0;
 }
+
+int
+test_sleep(box_function_ctx_t *ctx, const char *args, const char *args_end)
+{
+	(void) ctx;
+	(void) args;
+	(void) args_end;
+	/*
+	 * Sleep until an explicit wakeup. Purpose of this
+	 * function - test module unloading prevention while at
+	 * least one of its functions is being executed.
+	 */
+	fiber_sleep(100000);
+	return 0;
+}
diff --git a/test/box/gh-4648-func-load-unload.result b/test/box/gh-4648-func-load-unload.result
new file mode 100644
index 000000000..8c4d23938
--- /dev/null
+++ b/test/box/gh-4648-func-load-unload.result
@@ -0,0 +1,137 @@
+-- test-run result file version 2
+fiber = require('fiber')
+ | ---
+ | ...
+test_run = require('test_run').new()
+ | ---
+ | ...
+build_path = os.getenv("BUILDDIR")
+ | ---
+ | ...
+package.cpath = build_path..'/test/box/?.so;'..build_path..'/test/box/?.dylib;'..package.cpath
+ | ---
+ | ...
+errinj = box.error.injection
+ | ---
+ | ...
+
+--
+-- gh-4648: box.schema.func.drop() didn't unload a .so/.dylib
+-- module. Even if it was unused already. Moreover, recreation of
+-- functions from the same module led to its multiple mmaping.
+--
+
+current_module_count = errinj.get("ERRINJ_DYN_MODULE_COUNT")
+ | ---
+ | ...
+function check_module_count_diff(diff)                          \
+    local module_count = errinj.get("ERRINJ_DYN_MODULE_COUNT")  \
+    current_module_count = current_module_count + diff          \
+    if current_module_count ~= module_count then                \
+        return current_module_count, module_count               \
+    end                                                         \
+end
+ | ---
+ | ...
+
+-- Module is not loaded until any of its functions is called first
+-- time.
+box.schema.func.create('function1', {language = 'C'})
+ | ---
+ | ...
+check_module_count_diff(0)
+ | ---
+ | ...
+box.schema.func.drop('function1')
+ | ---
+ | ...
+check_module_count_diff(0)
+ | ---
+ | ...
+
+-- Module is unloaded when its function is dropped, and there are
+-- no not finished invocations of the function.
+box.schema.func.create('function1', {language = 'C'})
+ | ---
+ | ...
+check_module_count_diff(0)
+ | ---
+ | ...
+box.func.function1:call()
+ | ---
+ | ...
+check_module_count_diff(1)
+ | ---
+ | ...
+box.schema.func.drop('function1')
+ | ---
+ | ...
+check_module_count_diff(-1)
+ | ---
+ | ...
+
+-- A not finished invocation of a function from a module prevents
+-- its unload. Until the call is finished.
+box.schema.func.create('function1', {language = 'C'})
+ | ---
+ | ...
+box.schema.func.create('function1.test_sleep', {language = 'C'})
+ | ---
+ | ...
+check_module_count_diff(0)
+ | ---
+ | ...
+
+function long_call() box.func['function1.test_sleep']:call() end
+ | ---
+ | ...
+f1 = fiber.create(long_call)
+ | ---
+ | ...
+f2 = fiber.create(long_call)
+ | ---
+ | ...
+test_run:wait_cond(function()                                   \
+    return f1:status() == 'suspended' and                       \
+        f2:status() == 'suspended'                              \
+end)
+ | ---
+ | - true
+ | ...
+box.func.function1:call()
+ | ---
+ | ...
+check_module_count_diff(1)
+ | ---
+ | ...
+box.schema.func.drop('function1')
+ | ---
+ | ...
+box.schema.func.drop('function1.test_sleep')
+ | ---
+ | ...
+check_module_count_diff(0)
+ | ---
+ | ...
+
+f1:wakeup()
+ | ---
+ | ...
+test_run:wait_cond(function() return f1:status() == 'dead' end)
+ | ---
+ | - true
+ | ...
+check_module_count_diff(0)
+ | ---
+ | ...
+
+f2:wakeup()
+ | ---
+ | ...
+test_run:wait_cond(function() return f2:status() == 'dead' end)
+ | ---
+ | - true
+ | ...
+check_module_count_diff(-1)
+ | ---
+ | ...
diff --git a/test/box/gh-4648-func-load-unload.test.lua b/test/box/gh-4648-func-load-unload.test.lua
new file mode 100644
index 000000000..d7e445f65
--- /dev/null
+++ b/test/box/gh-4648-func-load-unload.test.lua
@@ -0,0 +1,63 @@
+fiber = require('fiber')
+test_run = require('test_run').new()
+build_path = os.getenv("BUILDDIR")
+package.cpath = build_path..'/test/box/?.so;'..build_path..'/test/box/?.dylib;'..package.cpath
+errinj = box.error.injection
+
+--
+-- gh-4648: box.schema.func.drop() didn't unload a .so/.dylib
+-- module. Even if it was unused already. Moreover, recreation of
+-- functions from the same module led to its multiple mmaping.
+--
+
+current_module_count = errinj.get("ERRINJ_DYN_MODULE_COUNT")
+function check_module_count_diff(diff)                          \
+    local module_count = errinj.get("ERRINJ_DYN_MODULE_COUNT")  \
+    current_module_count = current_module_count + diff          \
+    if current_module_count ~= module_count then                \
+        return current_module_count, module_count               \
+    end                                                         \
+end
+
+-- Module is not loaded until any of its functions is called first
+-- time.
+box.schema.func.create('function1', {language = 'C'})
+check_module_count_diff(0)
+box.schema.func.drop('function1')
+check_module_count_diff(0)
+
+-- Module is unloaded when its function is dropped, and there are
+-- no not finished invocations of the function.
+box.schema.func.create('function1', {language = 'C'})
+check_module_count_diff(0)
+box.func.function1:call()
+check_module_count_diff(1)
+box.schema.func.drop('function1')
+check_module_count_diff(-1)
+
+-- A not finished invocation of a function from a module prevents
+-- its unload. Until the call is finished.
+box.schema.func.create('function1', {language = 'C'})
+box.schema.func.create('function1.test_sleep', {language = 'C'})
+check_module_count_diff(0)
+
+function long_call() box.func['function1.test_sleep']:call() end
+f1 = fiber.create(long_call)
+f2 = fiber.create(long_call)
+test_run:wait_cond(function()                                   \
+    return f1:status() == 'suspended' and                       \
+        f2:status() == 'suspended'                              \
+end)
+box.func.function1:call()
+check_module_count_diff(1)
+box.schema.func.drop('function1')
+box.schema.func.drop('function1.test_sleep')
+check_module_count_diff(0)
+
+f1:wakeup()
+test_run:wait_cond(function() return f1:status() == 'dead' end)
+check_module_count_diff(0)
+
+f2:wakeup()
+test_run:wait_cond(function() return f2:status() == 'dead' end)
+check_module_count_diff(-1)
diff --git a/test/box/suite.ini b/test/box/suite.ini
index 6e8508188..9aa30ede6 100644
--- a/test/box/suite.ini
+++ b/test/box/suite.ini
@@ -3,7 +3,7 @@ core = tarantool
 description = Database tests
 script = box.lua
 disabled = rtree_errinj.test.lua tuple_bench.test.lua
-release_disabled = errinj.test.lua errinj_index.test.lua rtree_errinj.test.lua upsert_errinj.test.lua iproto_stress.test.lua
+release_disabled = errinj.test.lua errinj_index.test.lua rtree_errinj.test.lua upsert_errinj.test.lua iproto_stress.test.lua gh-4648-func-load-unload.test.lua
 lua_libs = lua/fifo.lua lua/utils.lua lua/bitset.lua lua/index_random_test.lua lua/push.lua lua/identifier.lua
 use_unix_sockets = True
 use_unix_sockets_iproto = True
-- 
2.21.0 (Apple Git-122.2)