From ca07088cd4ad45f04951504e0a5924af2de0129c Mon Sep 17 00:00:00 2001
From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Date: Tue, 26 Nov 2019 23:52:13 +0100
Subject: [PATCH] func: fix not unloading of unused modules
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 | 16 +++
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, 227 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 d9481b714d..431577127f 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 @@ module_load(const char *package, const char *package_end)
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 18b83faac3..581e468cba 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 3072a00ea6..672da21199 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 b0d983e2b5..87062d6a8f 100644
--- a/test/box/function1.c
+++ b/test/box/function1.c
@@ -229,3 +229,19 @@ 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 a cancellation. Purpose of this function -
+ * test module unloading prevention while at least one of
+ * its functions is being executed.
+ */
+ while (!fiber_is_cancelled())
+ fiber_sleep(0);
+ 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 0000000000..e695dd365d
--- /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:cancel()
+ | ---
+ | ...
+test_run:wait_cond(function() return f1:status() == 'dead' end)
+ | ---
+ | - true
+ | ...
+check_module_count_diff(0)
+ | ---
+ | ...
+
+f2:cancel()
+ | ---
+ | ...
+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 0000000000..10b9de87ac
--- /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:cancel()
+test_run:wait_cond(function() return f1:status() == 'dead' end)
+check_module_count_diff(0)
+
+f2:cancel()
+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 6e85081889..9aa30ede68 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
--
GitLab