From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp40.i.mail.ru (smtp40.i.mail.ru [94.100.177.100]) (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 2E3CA46970F for ; Thu, 28 Nov 2019 01:24:16 +0300 (MSK) References: <20191127084104.GD2752@atlas> From: Vladislav Shpilevoy Message-ID: Date: Wed, 27 Nov 2019 23:24:13 +0100 MIME-Version: 1.0 In-Reply-To: <20191127084104.GD2752@atlas> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [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: Konstantin Osipov , tarantool-patches@dev.tarantool.org Hi! Thanks for the review! On 27/11/2019 09:41, Konstantin Osipov wrote: > * Vladislav Shpilevoy [19/11/27 11:15]: >> 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. > > Wow. This is damn genius! > > It never occurred to me that error injections could be used to > test some obscure code paths. > > The impact of this on testing is going to be huge. >> 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); >> } > > This is LGTM. Georgy wrote the original patches so you may want > to solicit his review as well. >> +f1:wakeup() >> + | --- >> + | ... >> +test_run:wait_cond(function() return f1:status() == 'dead' end) >> + | --- >> + | - true >> + | ... >> +check_module_count_diff(0) >> + | --- >> + | ... >> + >> +f2:wakeup() > > This is a bit hackish way to cancel a sleep. I thought we had > errinj_sleep injection which would reliably cancel a sleep when > you clear it? If there is no such function I don't think > it is a blocker for this patch to get in as is. > The sleeping function is in a C module, and can use only public API from module.h. So we can't use error injections in it. But the sleep can be dropped in a different way. I pushed the diff below on the branch: ==================================================================== diff --git a/test/box/function1.c b/test/box/function1.c index fd434c280..87062d6a8 100644 --- a/test/box/function1.c +++ b/test/box/function1.c @@ -237,10 +237,11 @@ test_sleep(box_function_ctx_t *ctx, const char *args, const char *args_end) (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. + * Sleep until a cancellation. Purpose of this function - + * test module unloading prevention while at least one of + * its functions is being executed. */ - fiber_sleep(100000); + 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 index 8c4d23938..e695dd365 100644 --- a/test/box/gh-4648-func-load-unload.result +++ b/test/box/gh-4648-func-load-unload.result @@ -114,7 +114,7 @@ check_module_count_diff(0) | --- | ... -f1:wakeup() +f1:cancel() | --- | ... test_run:wait_cond(function() return f1:status() == 'dead' end) @@ -125,7 +125,7 @@ check_module_count_diff(0) | --- | ... -f2:wakeup() +f2:cancel() | --- | ... test_run:wait_cond(function() return f2:status() == 'dead' end) diff --git a/test/box/gh-4648-func-load-unload.test.lua b/test/box/gh-4648-func-load-unload.test.lua index d7e445f65..10b9de87a 100644 --- a/test/box/gh-4648-func-load-unload.test.lua +++ b/test/box/gh-4648-func-load-unload.test.lua @@ -54,10 +54,10 @@ box.schema.func.drop('function1') box.schema.func.drop('function1.test_sleep') check_module_count_diff(0) -f1:wakeup() +f1:cancel() test_run:wait_cond(function() return f1:status() == 'dead' end) check_module_count_diff(0) -f2:wakeup() +f2:cancel() test_run:wait_cond(function() return f2:status() == 'dead' end) check_module_count_diff(-1)