[Tarantool-patches] [PATCH v2] src/lua: fix running init lua script

[Leonid Vasiliev]

Hi!

Generally LGTM. Ping the next reviewer.

See some comments bellow:

I can't suggest a good enough component name for this patch, but
according to 9e45b41f1209f84fb8c18abae229196dfdacd367, "lua" is good
(without "src/").

On 14.12.2020 19:05, Artem Starshov wrote:
> When tarantool launched with -e flag and in
> script after there is an error, programm hangs.
> This happens because shed fiber launches separate
> fiber for init user script and starts auxiliary
> event loop. It's supposed that fiber will stop
> this loop, but in case of error in script, fiber
> tries to stop a loop when the last one isn't
> started yet.
> 
> Added a flag, which will watch is loop started and
> when fiber tries to call `ev_break()` we can be sure
> that loop is running already.
> 
> Fixes #4983
> ---
> @ChangeLog: `tarantool -e <script_with_error>` doesn't hang now.

I think it will be nice to add more information. Something like that:
"Fixed a hang when executing a script launched with the -e
option(`tarantool -e 'assert(false)'`), if an error was thrown before
the  yield."

> 
> Changes in v2:
>      - Test commit squashed with code fix commit;
>      - Fixed comments in test;
>      - Fixed commit message (no stairs);
>      - Closes -> Fixes;
>      - Removed small bugs in test;
>      - Moved test to tap.test(name, func)
> 
>   src/lua/init.c                                |  9 ++
>   .../gh-4983-tnt-e-assert-false-hangs.test.lua | 93 +++++++++++++++++++
>   2 files changed, 102 insertions(+)
>   create mode 100755 test/app-tap/gh-4983-tnt-e-assert-false-hangs.test.lua
> 
> diff --git a/src/lua/init.c b/src/lua/init.c
> index a0b2fc775..92bdb82c0 100644
> --- a/src/lua/init.c
> +++ b/src/lua/init.c
> @@ -569,6 +569,7 @@ run_script_f(va_list ap)
>   	 * never really dead. It never returns from its function.
>   	 */
>   	struct diag *diag = va_arg(ap, struct diag *);
> +	bool aux_loop_is_run = false;
>   
>   	/*
>   	 * Load libraries and execute chunks passed by -l and -e
> @@ -611,6 +612,7 @@ run_script_f(va_list ap)
>   	 * loop and re-schedule this fiber.
>   	 */
>   	fiber_sleep(0.0);
> +	aux_loop_is_run = true;
>   
>   	if (path && strcmp(path, "-") != 0 && access(path, F_OK) == 0) {
>   		/* Execute script. */
> @@ -650,6 +652,13 @@ run_script_f(va_list ap)
>   	 * return control back to tarantool_lua_run_script.
>   	 */
>   end:
> +	/*
> +	 * Auxiliary loop in tarantool_lua_run_script
> +	 * should start (ev_run()) before this fiber
> +	 * invokes ev_break().
> +	 */
> +	if (!aux_loop_is_run)
> +		fiber_sleep(0.0);
>   	ev_break(loop(), EVBREAK_ALL);
>   	return 0;
>   
> diff --git a/test/app-tap/gh-4983-tnt-e-assert-false-hangs.test.lua b/test/app-tap/gh-4983-tnt-e-assert-false-hangs.test.lua
> new file mode 100755
> index 000000000..27c631449
> --- /dev/null
> +++ b/test/app-tap/gh-4983-tnt-e-assert-false-hangs.test.lua
> @@ -0,0 +1,93 @@
> +#!/usr/bin/env tarantool
> +
> +local fiber = require('fiber')
> +local clock = require('clock')
> +local ffi = require('ffi')
> +local fio = require('fio')
> +local errno = require('errno')
> +local tap = require('tap')
> +
> +--
> +-- gh-4983: tarantool -e 'assert(false)' hangs
> +--
> +
> +-- For process_is_alive.
> +ffi.cdef([[
> +    int kill(pid_t pid, int sig);
> +]])
> +
> +---  Verify whether a process is alive.
> +local function process_is_alive(pid)
> +    local rc = ffi.C.kill(pid, 0)
> +    return rc == 0 or errno() ~= errno.ESRCH
> +end
> +
> +---  Return true if process completed before timeout.
> +---  Return false otherwise.

Only first line of the function description uses `---`.

> +local function wait_process_completion(pid, timeout)
> +    local start_time = clock.monotonic()
> +    local process_completed = false
> +    while clock.monotonic() - start_time < timeout do
> +        if not process_is_alive(pid) then
> +            process_completed = true
> +            break
> +        end
> +    end
> +    return process_completed
> +end
> +
> +--- Open file on reading with timeout.
> +local function open_with_timeout(filename, timeout)
> +    local fh
> +    local start_time = clock.monotonic()
> +    while not fh and clock.monotonic() - start_time < timeout do
> +        fh = fio.open(filename, {'O_RDONLY'})
> +    end
> +    return fh
> +end
> +
> +--- Try to read from file with timeout
> +--- with interval.

Description fits perfectly on one line.

> +local function read_with_timeout(fh, timeout, interval)
> +    local data = ''
> +    local start_time = clock.monotonic()
> +    while #data == 0 and clock.monotonic() - start_time < timeout do
> +        data = fh:read()
> +        if #data == 0 then fiber.sleep(interval) end
> +    end
> +    return data
> +end
> +
> +local TARANTOOL_PATH = arg[-1]
> +local output_file = fio.abspath('out.txt')
> +local line = ('%s -e "assert(false)" > %s 2>&1 & echo $!'):
> +                        format(TARANTOOL_PATH, output_file)
> +local process_waiting_timeout = 2.0
> +local file_read_timeout = 2.0
> +local file_read_interval = 0.2
> +local file_open_timeout = 2.0
> +
> +local res = tap.test('gh-4983-tnt-e-assert-false-hangs', function(test)
> +    test:plan(2)
> +
> +    local pid = tonumber(io.popen(line):read("*line"))
> +    assert(pid, "pid of proccess can't be recieved")
> +
> +    local process_completed = wait_process_completion(pid,
> +                                                      process_waiting_timeout)

According to https://dev.minetest.net/Lua_code_style_guidelines use two
tabs for indentation (also in our code I often see one tab). But don't
use C style indentation. The same in other places.

> +    test:ok(process_completed,
> +            ('tarantool process with pid = %d completed'):format(pid))
> +
> +    -- Kill process if hangs.
> +    if not process_completed then ffi.C.kill(pid, 9) end
> +
> +    local fh = open_with_timeout(output_file, file_open_timeout)
> +    assert(fh, 'error while opening ' .. output_file)
> +
> +    local data = read_with_timeout(fh, file_read_timeout, file_read_interval)
> +    test:like(data, "assertion failed", "assertion failure is displayed")
> +
> +    fh:close()
> +end)
> +
> +os.exit(res and 0 or 1)
> \ No newline at end of file

Add new line.

>