From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp45.i.mail.ru (smtp45.i.mail.ru [94.100.177.105]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 125DD46970F for ; Wed, 27 Nov 2019 02:29:46 +0300 (MSK) From: Vladislav Shpilevoy Date: Wed, 27 Nov 2019 00:29:41 +0100 Message-Id: In-Reply-To: References: MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Subject: [Tarantool-patches] [PATCH 2/2] func: fix not unloading of unused modules List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: tarantool-patches@dev.tarantool.org, kostja.osipov@gmail.com 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)