Re: [PATCH v2 2/6] Add functions to ease using Lua iterators from C

[Alexander Turenko <alexander.turenko@tarantool.org>]

Made several minor updates. I'll squash them into the original commit,
but show here separatelly to ease reading.

WBR, Alexander Turenko.

On Mon, Jan 28, 2019 at 09:17:16PM +0300, Alexander Turenko wrote:
> Updated a bit:
> 
> diff --git a/src/lua/utils.c b/src/lua/utils.c
> index 173d59a59..c8cfa70b3 100644
> --- a/src/lua/utils.c
> +++ b/src/lua/utils.c
> @@ -981,6 +981,11 @@ struct luaL_iterator *
>  luaL_iterator_new(lua_State *L, int idx)
>  {
>  	struct luaL_iterator *it = malloc(sizeof(struct luaL_iterator));
> +	if (it == NULL) {
> +		diag_set(OutOfMemory, sizeof(struct luaL_iterator),
> +			 "malloc", "luaL_iterator");
> +		return NULL;
> +	}
>  
>  	if (idx == 0) {
>  		/* gen, param, state are on top of a Lua stack. */

commit c7cf235b5ba68e911a1fd47e950fe205e963f9e7
Author: Alexander Turenko <alexander.turenko@tarantool.org>
Date:   Thu Feb 21 14:06:39 2019 +0300

    FIXUP: luaL_iterator: use global Lua state for unreferencing

diff --git a/src/lua/utils.c b/src/lua/utils.c
index 002c5e4b9..2e5096626 100644
--- a/src/lua/utils.c
+++ b/src/lua/utils.c
@@ -1052,11 +1052,11 @@ luaL_iterator_next(lua_State *L, struct luaL_iterator *it)
 	return nresults;
 }
 
-void luaL_iterator_delete(lua_State *L, struct luaL_iterator *it)
+void luaL_iterator_delete(struct luaL_iterator *it)
 {
-	luaL_unref(L, LUA_REGISTRYINDEX, it->gen);
-	luaL_unref(L, LUA_REGISTRYINDEX, it->param);
-	luaL_unref(L, LUA_REGISTRYINDEX, it->state);
+	luaL_unref(tarantool_L, LUA_REGISTRYINDEX, it->gen);
+	luaL_unref(tarantool_L, LUA_REGISTRYINDEX, it->param);
+	luaL_unref(tarantool_L, LUA_REGISTRYINDEX, it->state);
 	free(it);
 }
 
diff --git a/src/lua/utils.h b/src/lua/utils.h
index 772b5d877..c66a8a4b9 100644
--- a/src/lua/utils.h
+++ b/src/lua/utils.h
@@ -562,7 +562,7 @@ luaL_iterator_next(lua_State *L, struct luaL_iterator *it);
 /**
  * Free all resources held by the iterator.
  */
-void luaL_iterator_delete(lua_State *L, struct luaL_iterator *it);
+void luaL_iterator_delete(struct luaL_iterator *it);
 
 /* }}} */
 
diff --git a/test/unit/luaL_iterator.c b/test/unit/luaL_iterator.c
index 5a254f27d..038d34c5c 100644
--- a/test/unit/luaL_iterator.c
+++ b/test/unit/luaL_iterator.c
@@ -98,6 +98,13 @@ main()
 
 	struct lua_State *L = luaL_newstate();
 	luaL_openlibs(L);
+	tarantool_L = L;
+
+	/*
+	 * Check that everything works fine in a thread (a fiber)
+	 * other then the main one.
+	 */
+	L = lua_newthread(L);
 
 	/*
 	 * Expose luafun.
@@ -148,7 +155,7 @@ main()
 		is(lua_gettop(L) - top, 0, "%s: stack size", description);
 
 		/* Free the luaL_iterator structure. */
-		luaL_iterator_delete(L, it);
+		luaL_iterator_delete(it);
 
 		/* Check stack size. */
 		is(lua_gettop(L) - top, 0, "%s: stack size", description);

commit 1318e30c89efbb58de46fb5ed167cd61e3ff661a
Author: Alexander Turenko <alexander.turenko@tarantool.org>
Date:   Mon Feb 25 01:05:01 2019 +0300

    FIXUP: luaL_iterator: tweak a comment

diff --git a/src/lua/utils.h b/src/lua/utils.h
index c66a8a4b9..c4a16704f 100644
--- a/src/lua/utils.h
+++ b/src/lua/utils.h
@@ -543,7 +543,7 @@ struct luaL_iterator;
  * Create a Lua iterator from a gen, param, state triplet.
  *
  * If idx == 0, then three top stack values are used as the
- * triplet.
+ * triplet. Note: they are not popped.
  *
  * Otherwise idx is index on Lua stack points to a
  * {gen, param, state} table.

commit f95e07b340f4b698c2d6aa0ff6db437cc388ab85
Author: Alexander Turenko <alexander.turenko@tarantool.org>
Date:   Mon Feb 25 09:50:34 2019 +0300

    FIXUP: luaL_iterator: use pcall

diff --git a/src/lua/utils.c b/src/lua/utils.c
index 2e5096626..94f8c65d6 100644
--- a/src/lua/utils.c
+++ b/src/lua/utils.c
@@ -1029,7 +1029,15 @@ luaL_iterator_next(lua_State *L, struct luaL_iterator *it)
 	lua_rawgeti(L, LUA_REGISTRYINDEX, it->gen);
 	lua_rawgeti(L, LUA_REGISTRYINDEX, it->param);
 	lua_rawgeti(L, LUA_REGISTRYINDEX, it->state);
-	lua_call(L, 2, LUA_MULTRET);
+	if (luaT_call(L, 2, LUA_MULTRET) != 0) {
+		/*
+		 * Pop garbage from the call (a gen function
+		 * likely will not leave the stack even when raise
+		 * an error), pop a returned error.
+		 */
+		lua_settop(L, frame_start);
+		return -1;
+	}
 	int nresults = lua_gettop(L) - frame_start;
 
 	/*
diff --git a/src/lua/utils.h b/src/lua/utils.h
index c4a16704f..4a87dac45 100644
--- a/src/lua/utils.h
+++ b/src/lua/utils.h
@@ -553,8 +553,11 @@ luaL_iterator_new(lua_State *L, int idx);
 
 /**
  * Move iterator to the next value. Push values returned by
- * gen(param, state) and return its count. Zero means no more
- * results available.
+ * gen(param, state).
+ *
+ * Return count of pushed values. Zero means no more results
+ * available. In case of a Lua error in a gen function return -1
+ * and set a diag.
  */
 int
 luaL_iterator_next(lua_State *L, struct luaL_iterator *it);
diff --git a/test/unit/luaL_iterator.c b/test/unit/luaL_iterator.c
index 038d34c5c..8d25a0062 100644
--- a/test/unit/luaL_iterator.c
+++ b/test/unit/luaL_iterator.c
@@ -2,7 +2,12 @@
 #include <lauxlib.h>   /* luaL_*() */
 #include <lualib.h>    /* luaL_openlibs() */
 #include "unit.h"      /* plan, header, footer, is */
+#include "memory.h"    /* memory_init() */
+#include "fiber.h"     /* fiber_init() */
+#include "diag.h"      /* struct error, diag_*() */
+#include "exception.h" /* type_LuajitError */
 #include "lua/utils.h" /* luaL_iterator_*() */
+#include "lua/error.h" /* tarantool_lua_error_init() */
 
 extern char fun_lua[];
 
@@ -31,6 +36,8 @@ main()
 		int idx;
 		/* How much values are in the iterator. */
 		int iterations;
+		/* Expected error (if any). */
+		const char *exp_err;
 	} cases[] = {
 		{
 			.description = "pairs, zero idx",
@@ -39,6 +46,7 @@ main()
 			.first_value = 42,
 			.idx = 0,
 			.iterations = 1,
+			.exp_err = NULL,
 		},
 		{
 			.description = "ipairs, zero idx",
@@ -47,6 +55,7 @@ main()
 			.first_value = 42,
 			.idx = 0,
 			.iterations = 3,
+			.exp_err = NULL,
 		},
 		{
 			.description = "luafun iterator, zero idx",
@@ -55,6 +64,7 @@ main()
 			.first_value = 42,
 			.idx = 0,
 			.iterations = 3,
+			.exp_err = NULL,
 		},
 		{
 			.description = "pairs, from table",
@@ -63,6 +73,7 @@ main()
 			.first_value = 42,
 			.idx = -1,
 			.iterations = 1,
+			.exp_err = NULL,
 		},
 		{
 			.description = "ipairs, from table",
@@ -71,6 +82,7 @@ main()
 			.first_value = 42,
 			.idx = -1,
 			.iterations = 3,
+			.exp_err = NULL,
 		},
 		{
 			.description = "luafun iterator, from table",
@@ -79,19 +91,34 @@ main()
 			.first_value = 42,
 			.idx = -1,
 			.iterations = 3,
+			.exp_err = NULL,
+		},
+		{
+			.description = "lua error",
+			.init = "return error, 'I am the error', 0",
+			.init_retvals = 3,
+			.first_value = 0,
+			.idx = 0,
+			.iterations = 0,
+			.exp_err = "I am the error",
 		},
 	};
 
 	int cases_cnt = (int) (sizeof(cases) / sizeof(cases[0]));
 	/*
-	 * * Check stack size after creating luaL_iterator (triple
-	 *   times).
 	 * * 4 checks per iteration.
-	 * * Check that values ends.
+	 * * 3 checks of a stack size.
+	 * * 1 check that values ends (for success cases).
+	 * * 1 check for an iterator error (for error cases).
+	 * * 1 check for an error type (for error cases).
+	 * * 1 check for an error message (for error cases).
 	 */
 	int planned = 0;
-	for (int i = 0; i < cases_cnt; ++i)
+	for (int i = 0; i < cases_cnt; ++i) {
 		planned += cases[i].iterations * 4 + 4;
+		if (cases[i].exp_err != NULL)
+			planned += 2;
+	}
 
 	plan(planned);
 	header();
@@ -100,6 +127,10 @@ main()
 	luaL_openlibs(L);
 	tarantool_L = L;
 
+	memory_init();
+	fiber_init(fiber_c_invoke);
+	tarantool_lua_error_init(L);
+
 	/*
 	 * Check that everything works fine in a thread (a fiber)
 	 * other then the main one.
@@ -147,9 +178,20 @@ main()
 			   description, j);
 		}
 
-		/* Check the iterator ends when expected. */
-		int rc = luaL_iterator_next(L, it);
-		is(rc, 0, "%s: iterator ends", description);
+		if (cases[i].exp_err == NULL) {
+			/* Check the iterator ends when expected. */
+			int rc = luaL_iterator_next(L, it);
+			is(rc, 0, "%s: iterator ends", description);
+		} else {
+			/* Check expected error. */
+			int rc = luaL_iterator_next(L, it);
+			is(rc, -1, "%s: iterator error", description);
+			struct error *e = diag_last_error(diag_get());
+			is(e->type, &type_LuajitError, "%s: check error type",
+			   description);
+			ok(!strcmp(e->errmsg, cases[i].exp_err),
+			   "%s: check error message", description);
+		}
 
 		/* Check stack size. */
 		is(lua_gettop(L) - top, 0, "%s: stack size", description);
diff --git a/test/unit/luaL_iterator.result b/test/unit/luaL_iterator.result
index f4eda5695..2472eedcf 100644
--- a/test/unit/luaL_iterator.result
+++ b/test/unit/luaL_iterator.result
@@ -1,4 +1,4 @@
-1..80
+1..86
 	*** main ***
 ok 1 - pairs, zero idx: stack size
 ok 2 - pairs, zero idx: iter 0: gen() retval count
@@ -80,4 +80,10 @@ ok 77 - luafun iterator, from table: iter: 2: stack size
 ok 78 - luafun iterator, from table: iterator ends
 ok 79 - luafun iterator, from table: stack size
 ok 80 - luafun iterator, from table: stack size
+ok 81 - lua error: stack size
+ok 82 - lua error: iterator error
+ok 83 - lua error: check error type
+ok 84 - lua error: check error message
+ok 85 - lua error: stack size
+ok 86 - lua error: stack size
 	*** main: done ***

commit 6edef55a8f689f595a760a9c06180de7da37976e
Author: Alexander Turenko <alexander.turenko@tarantool.org>
Date:   Fri Mar 1 02:27:25 2019 +0300

    FIXUP: luaL_iterator: after rebase fix

diff --git a/test/unit/CMakeLists.txt b/test/unit/CMakeLists.txt
index 263856a7a..663ed2ec2 100644
--- a/test/unit/CMakeLists.txt
+++ b/test/unit/CMakeLists.txt
@@ -139,9 +139,9 @@ target_link_libraries(histogram.test stat unit)
 add_executable(ratelimit.test ratelimit.c)
 target_link_libraries(ratelimit.test unit)
 add_executable(luaL_iterator.test luaL_iterator.c)
-target_link_libraries(luaL_iterator.test unit server core misc
+target_link_libraries(luaL_iterator.test unit server coll core misc
     ${CURL_LIBRARIES} ${LIBYAML_LIBRARIES} ${READLINE_LIBRARIES}
-    ${ICU_LIBRARIES} ${LUAJIT_LIBRARIES})
+    ${ICU_LIBRARIES} ${LUAJIT_LIBRARIES} dl)
 
 add_executable(say.test say.c)
 target_link_libraries(say.test core unit)