Changes in v2: - changed commit message name - added 'follows up #4983' - removed collecting details about process due to Linux specific - left only the last commit because the previous was pushed to master and the first one is empty after the review - added comment describing the reason of using timeout module in presence of built-in 'popen' module Changes in v3: - split one commit into two: first - increase timeout, second - move helper functions to separated module Changes in v4: - split two commits into 3: first - increase timeout, second - add module process_timeout, third - modify -e assert(false) test using process_timeout. Creating module `process_timeout` in separate commit is convenient for working under other tests using this module while this patch isn't pushed on master. Artem Starshov (3): test: change timeout in -e assert(false) test test: add separated module for proccess operations with timeout test: change -e assert(false) test using process_timeout module .../gh-4983-tnt-e-assert-false-hangs.test.lua | 70 ++++--------------- test/app-tap/lua/process_timeout.lua | 59 ++++++++++++++++ test/app-tap/suite.ini | 2 +- 3 files changed, 74 insertions(+), 57 deletions(-) create mode 100644 test/app-tap/lua/process_timeout.lua -- 2.28.0
Set timeout to 60 sec for waiting operations with process, like in default timeout for `cond_wait` in test-run. Follows up #4983 --- test/app-tap/gh-4983-tnt-e-assert-false-hangs.test.lua | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) 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 index 656fe212b..c8313770e 100755 --- 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 @@ -60,10 +60,12 @@ 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 = 30.0 -local file_read_timeout = 30.0 + +-- Like a default timeout for `cond_wait` in test-run +local process_waiting_timeout = 60.0 +local file_read_timeout = 60.0 +local file_open_timeout = 60.0 local file_read_interval = 0.2 -local file_open_timeout = 30.0 local res = tap.test('gh-4983-tnt-e-assert-false-hangs', function(test) test:plan(2) -- 2.28.0
Functions with timeout transferred to module test/app-tap/lua/process_timeout.lua for further using in other tests. (from test/app-tap/gh-4983-tnt-e-assert-false-hangs.test.lua) Follows up #4983 --- test/app-tap/lua/process_timeout.lua | 59 ++++++++++++++++++++++++++++ test/app-tap/suite.ini | 2 +- 2 files changed, 60 insertions(+), 1 deletion(-) create mode 100644 test/app-tap/lua/process_timeout.lua diff --git a/test/app-tap/lua/process_timeout.lua b/test/app-tap/lua/process_timeout.lua new file mode 100644 index 000000000..1ca3636b9 --- /dev/null +++ b/test/app-tap/lua/process_timeout.lua @@ -0,0 +1,59 @@ +local fiber = require('fiber') +local clock = require('clock') +local ffi = require('ffi') +local fio = require('fio') +local errno = require('errno') + +-- 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 + +--- Returns true if process completed before timeout, 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 + fiber.sleep(0.01) + 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'}) + fiber.sleep(0.01) + 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 + +return { + process_is_alive = process_is_alive, + wait_process_completion = wait_process_completion, + open_with_timeout = open_with_timeout, + read_with_timeout = read_with_timeout +} diff --git a/test/app-tap/suite.ini b/test/app-tap/suite.ini index 2f16128f9..6e27f88d6 100644 --- a/test/app-tap/suite.ini +++ b/test/app-tap/suite.ini @@ -1,7 +1,7 @@ [default] core = app description = application server tests (TAP) -lua_libs = lua/require_mod.lua lua/serializer_test.lua +lua_libs = lua/require_mod.lua lua/serializer_test.lua lua/process_timeout.lua is_parallel = True pretest_clean = True use_unix_sockets_iproto = True -- 2.28.0
Removed definitions of functions with timeout. Now they are used from separated module process_timeout.lua Follows up #4983 --- .../gh-4983-tnt-e-assert-false-hangs.test.lua | 62 +++---------------- 1 file changed, 9 insertions(+), 53 deletions(-) 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 index c8313770e..20046d7a5 100755 --- 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 @@ -1,61 +1,17 @@ #!/usr/bin/env tarantool -local fiber = require('fiber') -local clock = require('clock') -local ffi = require('ffi') -local fio = require('fio') -local errno = require('errno') +-- Using io.popen and self written module process_timeout +-- in presence of the 'popen' built-in module because the +-- last one isn't available in 1.10 +local process_timeout = require('process_timeout') local tap = require('tap') +local fio = require('fio') +local ffi = require('ffi') -- -- 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 - ---- Returns true if process completed before timeout, 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 - 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 $!'): @@ -73,7 +29,7 @@ local res = tap.test('gh-4983-tnt-e-assert-false-hangs', function(test) local pid = tonumber(io.popen(line):read('*line')) assert(pid, 'pid of proccess can\'t be recieved') - local process_completed = wait_process_completion(pid, + local process_completed = process_timeout.wait_process_completion(pid, process_waiting_timeout) test:ok(process_completed, @@ -82,10 +38,10 @@ local res = tap.test('gh-4983-tnt-e-assert-false-hangs', function(test) -- 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) + local fh = process_timeout.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) + local data = process_timeout.read_with_timeout(fh, file_read_timeout, file_read_interval) test:like(data, 'assertion failed', 'assertion failure is displayed') fh:close() -- 2.28.0
P.S.: Branch:https://github.com/tarantool/tarantool/tree/artemreyt/gh-4983-fix-test 18.01.2021 15:45, Artem Starshov пишет: > Changes in v2: > - changed commit message name > - added 'follows up #4983' > - removed collecting details about process due to Linux specific > - left only the last commit because the previous was pushed to master > and the first one is empty after the review > - added comment describing the reason of using timeout module in > presence of built-in 'popen' module > > Changes in v3: > - split one commit into two: first - increase timeout, second - move > helper functions to separated module > > Changes in v4: > - split two commits into 3: first - increase timeout, second - add module > process_timeout, third - modify -e assert(false) test using process_timeout. > Creating module `process_timeout` in separate commit is convenient for working > under other tests using this module while this patch isn't pushed on master. > > Artem Starshov (3): > test: change timeout in -e assert(false) test > test: add separated module for proccess operations with timeout > test: change -e assert(false) test using process_timeout module > > .../gh-4983-tnt-e-assert-false-hangs.test.lua | 70 ++++--------------- > test/app-tap/lua/process_timeout.lua | 59 ++++++++++++++++ > test/app-tap/suite.ini | 2 +- > 3 files changed, 74 insertions(+), 57 deletions(-) > create mode 100644 test/app-tap/lua/process_timeout.lua >
Thanks for the patch!
I think it would be more clear if you will add reasons
why we need to reduce timeout for two times.
On 18.01.2021 15:45, Artem Starshov wrote:
> Set timeout to 60 sec for waiting operations with process,
> like in default timeout for `cond_wait` in test-run.
>
> Follows up #4983
> ---
> test/app-tap/gh-4983-tnt-e-assert-false-hangs.test.lua | 8 +++++---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> 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
> index 656fe212b..c8313770e 100755
> --- 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
> @@ -60,10 +60,12 @@ 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 = 30.0
> -local file_read_timeout = 30.0
> +
> +-- Like a default timeout for `cond_wait` in test-run
> +local process_waiting_timeout = 60.0
> +local file_read_timeout = 60.0
> +local file_open_timeout = 60.0
> local file_read_interval = 0.2
> -local file_open_timeout = 30.0
>
> local res = tap.test('gh-4983-tnt-e-assert-false-hangs', function(test)
> test:plan(2)
Thanks for the patch!
LGTM
I see no reasons to chaneg for latest two patches.
At least review would be much more easier with a single patch.
On 18.01.2021 15:45, Artem Starshov wrote:
> Removed definitions of functions with timeout. Now they are used
> from separated module process_timeout.lua
>
> Follows up #4983
> ---
> .../gh-4983-tnt-e-assert-false-hangs.test.lua | 62 +++----------------
> 1 file changed, 9 insertions(+), 53 deletions(-)
>
> 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
> index c8313770e..20046d7a5 100755
> --- 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
> @@ -1,61 +1,17 @@
> #!/usr/bin/env tarantool
>
> -local fiber = require('fiber')
> -local clock = require('clock')
> -local ffi = require('ffi')
> -local fio = require('fio')
> -local errno = require('errno')
> +-- Using io.popen and self written module process_timeout
> +-- in presence of the 'popen' built-in module because the
> +-- last one isn't available in 1.10
> +local process_timeout = require('process_timeout')
> local tap = require('tap')
> +local fio = require('fio')
> +local ffi = require('ffi')
>
> --
> -- 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
> -
> ---- Returns true if process completed before timeout, 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
> - 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 $!'):
> @@ -73,7 +29,7 @@ local res = tap.test('gh-4983-tnt-e-assert-false-hangs', function(test)
> local pid = tonumber(io.popen(line):read('*line'))
> assert(pid, 'pid of proccess can\'t be recieved')
>
> - local process_completed = wait_process_completion(pid,
> + local process_completed = process_timeout.wait_process_completion(pid,
> process_waiting_timeout)
>
> test:ok(process_completed,
> @@ -82,10 +38,10 @@ local res = tap.test('gh-4983-tnt-e-assert-false-hangs', function(test)
> -- 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)
> + local fh = process_timeout.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)
> + local data = process_timeout.read_with_timeout(fh, file_read_timeout, file_read_interval)
> test:like(data, 'assertion failed', 'assertion failure is displayed')
>
> fh:close()
LGTM
On 18.01.2021 15:45, Artem Starshov wrote:
> Changes in v2:
> - changed commit message name
> - added 'follows up #4983'
> - removed collecting details about process due to Linux specific
> - left only the last commit because the previous was pushed to master
> and the first one is empty after the review
> - added comment describing the reason of using timeout module in
> presence of built-in 'popen' module
>
> Changes in v3:
> - split one commit into two: first - increase timeout, second - move
> helper functions to separated module
>
> Changes in v4:
> - split two commits into 3: first - increase timeout, second - add module
> process_timeout, third - modify -e assert(false) test using process_timeout.
> Creating module `process_timeout` in separate commit is convenient for working
> under other tests using this module while this patch isn't pushed on master.
>
> Artem Starshov (3):
> test: change timeout in -e assert(false) test
> test: add separated module for proccess operations with timeout
> test: change -e assert(false) test using process_timeout module
>
> .../gh-4983-tnt-e-assert-false-hangs.test.lua | 70 ++++---------------
> test/app-tap/lua/process_timeout.lua | 59 ++++++++++++++++
> test/app-tap/suite.ini | 2 +-
> 3 files changed, 74 insertions(+), 57 deletions(-)
> create mode 100644 test/app-tap/lua/process_timeout.lua
>
Creating module `process_timeout` in separate commit is convenient for working
under other tests using this module while this patch isn't pushed on master.
(from cover letter)
21.01.2021 19:23, Sergey Bronnikov пишет:
> I see no reasons to chaneg for latest two patches.
Hi, thanks for review! 21.01.2021 19:20, Sergey Bronnikov пишет: > Thanks for the patch! > > I think it would be more clear if you will add reasons > > why we need to reduce timeout for two times. Done. > > On 18.01.2021 15:45, Artem Starshov wrote: >> Set timeout to 60 sec for waiting operations with process, >> like in default timeout for `cond_wait` in test-run. >> >> Follows up #4983 >> --- >> test/app-tap/gh-4983-tnt-e-assert-false-hangs.test.lua | 8 +++++--- >> 1 file changed, 5 insertions(+), 3 deletions(-) >> >> 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 >> index 656fe212b..c8313770e 100755 >> --- 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 >> @@ -60,10 +60,12 @@ 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 = 30.0 >> -local file_read_timeout = 30.0 >> + >> +-- Like a default timeout for `cond_wait` in test-run >> +local process_waiting_timeout = 60.0 >> +local file_read_timeout = 60.0 >> +local file_open_timeout = 60.0 >> local file_read_interval = 0.2 >> -local file_open_timeout = 30.0 >> local res = tap.test('gh-4983-tnt-e-assert-false-hangs', >> function(test) >> test:plan(2)
Hello,
On 18 янв 15:45, Artem Starshov via Tarantool-patches wrote:
> Changes in v2:
> - changed commit message name
> - added 'follows up #4983'
> - removed collecting details about process due to Linux specific
> - left only the last commit because the previous was pushed to master
> and the first one is empty after the review
> - added comment describing the reason of using timeout module in
> presence of built-in 'popen' module
>
> Changes in v3:
> - split one commit into two: first - increase timeout, second - move
> helper functions to separated module
>
> Changes in v4:
> - split two commits into 3: first - increase timeout, second - add module
> process_timeout, third - modify -e assert(false) test using process_timeout.
> Creating module `process_timeout` in separate commit is convenient for working
> under other tests using this module while this patch isn't pushed on master.
>
> Artem Starshov (3):
> test: change timeout in -e assert(false) test
> test: add separated module for proccess operations with timeout
> test: change -e assert(false) test using process_timeout module
I've checked your patchset into 1.10, 2.6, 2.7 and master.
--
Regards, Kirill Yukhin