Re: [Tarantool-patches] [PATCH v12 2/8] module_cache: move module handling into own subsystem

[Cyrill Gorcunov via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

On Sun, Jan 24, 2021 at 05:26:48PM +0100, Vladislav Shpilevoy wrote:
> Thanks for the patch!
> 
> See 4 comments below.
> 
> On 18.01.2021 21:35, Cyrill Gorcunov wrote:
> > The module handling should not be bound to particular
> > function implementation (we will have two users: already
> > existing functions for "_func" space, and a new upcoming
> > one which will be serving cbox submodule in next patch).
> 
> 1. It is now cmod, not cbox.

Thanks!

> > +
> > +#include "box/error.h"
> 
> 2. #include "error.h" and #include "box/error.h" are the same
> files. Because you are in box/ folder.

OK

> > +
> > +	mod_sym->addr = module_sym(module, name.sym);
> > +	if (mod_sym->addr == NULL)
> > +		return -1;
> 
> 3. Is it correct, that you second time has deleted the bugfix
> which you did about module not being unloaded when first
> symbol load fails?

Sigh... This sneaky issue triggers for n'th time :( Thanks
for catching, Vlad!

> > +#pragma once
> > +
> > +#include <small/rlist.h>
> 
> 4. Please, use "" instead of <> for non-system headers.
OK, an update on top of the patch. I'll squash it later
---
 src/box/module_cache.c | 30 ++++++++++++++++++++++++++----
 src/box/module_cache.h |  2 +-
 2 files changed, 27 insertions(+), 5 deletions(-)

diff --git a/src/box/module_cache.c b/src/box/module_cache.c
index 9fe316807..a66b84efb 100644
--- a/src/box/module_cache.c
+++ b/src/box/module_cache.c
@@ -16,7 +16,7 @@
 #include "fiber.h"
 #include "port.h"
 
-#include "box/error.h"
+#include "error.h"
 #include "lua/utils.h"
 #include "libeio/eio.h"
 
@@ -338,8 +338,14 @@ module_sym_load(struct module_sym *mod_sym)
 	struct func_name name;
 	func_split_name(mod_sym->name, &name);
 
-	struct module *module = module_cache_find(name.package, name.package_end);
-	if (module == NULL) {
+	/*
+	 * In case if module has been loaded already by
+	 * some previous call we can eliminate redundant
+	 * loading and take it from the cache.
+	 */
+	struct module *cached, *module;
+	cached = module_cache_find(name.package, name.package_end);
+	if (cached == NULL) {
 		module = module_load(name.package, name.package_end);
 		if (module == NULL)
 			return -1;
@@ -347,11 +353,27 @@ module_sym_load(struct module_sym *mod_sym)
 			module_delete(module);
 			return -1;
 		}
+	} else {
+		module = cached;
 	}
 
 	mod_sym->addr = module_sym(module, name.sym);
-	if (mod_sym->addr == NULL)
+	if (mod_sym->addr == NULL) {
+		if (cached == NULL) {
+			/*
+			 * In case if it was a first load we should
+			 * clean the cache immediately otherwise
+			 * the module continue being referenced even
+			 * if there will be no use of it.
+			 *
+			 * Note the module_sym set an error thus be
+			 * careful to not wipe it.
+			 */
+			module_cache_del(name.package, name.package_end);
+			module_delete(module);
+		}
 		return -1;
+	}
 
 	mod_sym->module = module;
 	rlist_add(&module->funcs_list, &mod_sym->item);
diff --git a/src/box/module_cache.h b/src/box/module_cache.h
index fd789f603..0f5d2b64a 100644
--- a/src/box/module_cache.h
+++ b/src/box/module_cache.h
@@ -6,7 +6,7 @@
 
 #pragma once
 
-#include <small/rlist.h>
+#include "small/rlist.h"
 
 #if defined(__cplusplus)
 extern "C" {
-- 
2.29.2