From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org> To: Konstantin Osipov <kostja.osipov@gmail.com>, tarantool-patches@dev.tarantool.org Subject: Re: [Tarantool-patches] [PATCH 2/2] func: fix not unloading of unused modules Date: Wed, 27 Nov 2019 23:24:13 +0100 [thread overview] Message-ID: <b2ef9238-3f1a-afc6-062a-aeb265419bb5@tarantool.org> (raw) In-Reply-To: <20191127084104.GD2752@atlas> Hi! Thanks for the review! On 27/11/2019 09:41, Konstantin Osipov wrote: > * Vladislav Shpilevoy <v.shpilevoy@tarantool.org> [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)
next prev parent reply other threads:[~2019-11-27 22:24 UTC|newest] Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-11-26 23:29 [Tarantool-patches] [PATCH 0/2] Module is not unloaded on function drop Vladislav Shpilevoy 2019-11-26 23:29 ` [Tarantool-patches] [PATCH 1/2] errinj: provide 'get' method in Lua Vladislav Shpilevoy 2019-11-27 8:35 ` Konstantin Osipov 2019-11-26 23:29 ` [Tarantool-patches] [PATCH 2/2] func: fix not unloading of unused modules Vladislav Shpilevoy 2019-11-27 8:41 ` Konstantin Osipov 2019-11-27 22:24 ` Vladislav Shpilevoy [this message] 2019-12-10 14:22 ` [Tarantool-patches] [PATCH 0/2] Module is not unloaded on function drop Kirill Yukhin 2019-12-11 21:36 ` Alexander Turenko 2019-12-11 22:33 ` Vladislav Shpilevoy 2019-12-19 4:50 ` Kirill Yukhin
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=b2ef9238-3f1a-afc6-062a-aeb265419bb5@tarantool.org \ --to=v.shpilevoy@tarantool.org \ --cc=kostja.osipov@gmail.com \ --cc=tarantool-patches@dev.tarantool.org \ --subject='Re: [Tarantool-patches] [PATCH 2/2] func: fix not unloading of unused modules' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox