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

[Vladislav Shpilevoy]

Hi! Thanks for the review!

On 27/11/2019 09:41, Konstantin Osipov wrote:
> * Vladislav Shpilevoy <v.shpilevoy at 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)