[Tarantool-patches] [PATCH 0/2] tarantool -e 'assert(false)' hangs

[Tarantool-patches] [PATCH 0/2] tarantool -e 'assert(false)' hangs

[Artem Starshov]

Branch: https://github.com/tarantool/tarantool/tree/artemreyt/gh-4983-tnt-e-assert-false-hangs
Issue: https://github.com/tarantool/tarantool/issues/4983

Artem Starshov (2):
  src/lua: fix running init lua script
  test: add test for tarantool -e assert(false)

 src/lua/init.c                                |  9 ++
 .../gh-4983-tnt-e-assert-false-hangs.test.lua | 99 +++++++++++++++++++
 2 files changed, 108 insertions(+)
 create mode 100755 test/app-tap/gh-4983-tnt-e-assert-false-hangs.test.lua

-- 
2.28.0

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

[Artem Starshov]

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 runnig already.

Part of #4983
---
 src/lua/init.c | 9 +++++++++
 1 file changed, 9 insertions(+)

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;
 
-- 
2.28.0

Re: [Tarantool-patches] [PATCH 1/2] src/lua: fix running init lua script

[Leonid Vasiliev]

Hi! Thank you for the patch.
LGTM

On 09.12.2020 22:28, 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.
Nice stairs)

> 
> Added a flag, which will watch is loop started and
> when fiber tries to call `ev_break()` we can be sure
> that loop is runnig already.
> 
> Part of #4983

Closes.

> ---
>   src/lua/init.c | 9 +++++++++
>   1 file changed, 9 insertions(+)
> 
> 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;
>   
> 

Re: [Tarantool-patches] [PATCH 1/2] src/lua: fix running init lua script

[Leonid Vasiliev]

>> Part of #4983
> 
> Closes.

Fixes

[Tarantool-patches] [PATCH 2/2] test: add test for tarantool -e assert(false)

[Artem Starshov]

Closes #4983
---
 .../gh-4983-tnt-e-assert-false-hangs.test.lua | 99 +++++++++++++++++++
 1 file changed, 99 insertions(+)
 create mode 100755 test/app-tap/gh-4983-tnt-e-assert-false-hangs.test.lua

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..2213b1363
--- /dev/null
+++ b/test/app-tap/gh-4983-tnt-e-assert-false-hangs.test.lua
@@ -0,0 +1,99 @@
+#!/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')
+local test = tap.test('gh-4983-tnt-e-assert-false-hangs')
+
+--
+-- 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.
+--
+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
+        clock.monotonic()
+    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.
+--
+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
+
+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)
+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()
+test:check()
-- 
2.28.0

Re: [Tarantool-patches] [PATCH 2/2] test: add test for tarantool -e assert(false)

[Leonid Vasiliev]

Hi! Thank you for the patch.

The code is fine but I have some questions:

On 09.12.2020 22:28, Artem Starshov wrote:
> Closes #4983

AFAIK test usually added to the same commit with fixes. If there is no
reason to send the test in separate commit. In this case, the patch is
easier to backport.
If you are adding a test by a separate commit, mark it "Follows up".

> ---
>   .../gh-4983-tnt-e-assert-false-hangs.test.lua | 99 +++++++++++++++++++
>   1 file changed, 99 insertions(+)
>   create mode 100755 test/app-tap/gh-4983-tnt-e-assert-false-hangs.test.lua
> 
> 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..2213b1363
> --- /dev/null
> +++ b/test/app-tap/gh-4983-tnt-e-assert-false-hangs.test.lua
> @@ -0,0 +1,99 @@
> +#!/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')

Please, add a blank line to separate `require...` from code.

> +local test = tap.test('gh-4983-tnt-e-assert-false-hangs')
> +
> +--
> +-- gh-4983: tarantool -e 'assert(false)' hangs
> +--
> +
> +--
> +-- For process_is_alive.
> +--

What style guide did you get this version of comments from?
(Here and below)
--
-- comment
--

Versions known to me:
-- comment

or

--- Function description
-- some trololo

> +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.
> +--
> +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
> +        clock.monotonic()

What for this `clock.monotonic()`?

> +    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.
> +--
> +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
> +

Up to you. I propose to move all this stuff into a separate
function (exclude os.exit...).
Maybe like it's done in "gh-4761-json-per-call-options.test.lua".

> +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
> +
> +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)
> +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()
> +test:check()

os.exit(test:check() and 0 or 1)

> 

Re: [Tarantool-patches] [PATCH 2/2] test: add test for tarantool -e assert(false)

[Artem]

Hi, thanks for the review!

See my 5 comments below.

10.12.2020 21:11, Leonid Vasiliev пишет:
> Hi! Thank you for the patch.
>
> The code is fine but I have some questions:
>
> On 09.12.2020 22:28, Artem Starshov wrote:
>> Closes #4983
>
> AFAIK test usually added to the same commit with fixes. If there is no
> reason to send the test in separate commit. In this case, the patch is
> easier to backport.
> If you are adding a test by a separate commit, mark it "Follows up".
>
1. > the patch is easier to backport.

Agree with this statement. There will be patch v2 with one commit.

>> ---
>>   .../gh-4983-tnt-e-assert-false-hangs.test.lua | 99 +++++++++++++++++++
>>   1 file changed, 99 insertions(+)
>>   create mode 100755 
>> test/app-tap/gh-4983-tnt-e-assert-false-hangs.test.lua
>>
>> 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..2213b1363
>> --- /dev/null
>> +++ b/test/app-tap/gh-4983-tnt-e-assert-false-hangs.test.lua
>> @@ -0,0 +1,99 @@
>> +#!/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')
>
> Please, add a blank line to separate `require...` from code.
2. Done.
>
>> +local test = tap.test('gh-4983-tnt-e-assert-false-hangs')
>> +
>> +--
>> +-- gh-4983: tarantool -e 'assert(false)' hangs
>> +--
>> +
>> +--
>> +-- For process_is_alive.
>> +--
>
> What style guide did you get this version of comments from?
> (Here and below)
> -- 
> -- comment
> -- 

3. I took it from popen.test.lua and tap.test.lua. But there are a lot 
of variants

of commenting in tests code.

> Versions known to me:
> -- comment
>
> or
>
> --- Function description
> -- some trololo
>
4. Chose this variant(--- Function description), because in some editors 
it will help to

read description for functions.

>> +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.
>> +--
>> +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
>> +        clock.monotonic()
>
> What for this `clock.monotonic()`?
>
It's my mistake. Removed.
>> +    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.
>> +--
>> +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
>> +
>
> Up to you. I propose to move all this stuff into a separate
> function (exclude os.exit...).
> Maybe like it's done in "gh-4761-json-per-call-options.test.lua".
>
5. Done.
>> +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
>> +
>> +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)
>> +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()
>> +test:check()
>
> os.exit(test:check() and 0 or 1)
>
>>

Re: [Tarantool-patches] [PATCH 0/2] tarantool -e 'assert(false)' hangs

[Leonid Vasiliev]

Hi! Thank you for the patch.

Add @ChangeLog.

On 09.12.2020 22:28, Artem Starshov wrote:
> Branch: https://github.com/tarantool/tarantool/tree/artemreyt/gh-4983-tnt-e-assert-false-hangs
> Issue: https://github.com/tarantool/tarantool/issues/4983
> 
> Artem Starshov (2):
>    src/lua: fix running init lua script
>    test: add test for tarantool -e assert(false)
> 
>   src/lua/init.c                                |  9 ++
>   .../gh-4983-tnt-e-assert-false-hangs.test.lua | 99 +++++++++++++++++++
>   2 files changed, 108 insertions(+)
>   create mode 100755 test/app-tap/gh-4983-tnt-e-assert-false-hangs.test.lua
>