Message ID | 20210507103258.232174-3-lkml@jv-coder.de |
---|---|
State | Superseded |
Headers | show |
Series | shell: Fix orphan timeout sleep processes | expand |
Hi Joerg,
Reviewed-by: Li Wang <liwang@redhat.com>
The patchset looks good except for few minor issues, we can help fix
them when merging.
--- a/lib/newlib_tests/shell/timeout03.sh
+++ b/lib/newlib_tests/shell/timeout03.sh
@@ -32,7 +32,7 @@ do_test()
{
tst_res TINFO "testing killing test after TST_TIMEOUT"
- tst_sleep 2
+ sleep 2
tst_res TFAIL "test: running after TST_TIMEOUT"
}
@@ -40,7 +40,7 @@ cleanup()
{
tst_res TPASS "test run cleanup after timeout"
- tst_sleep 15 # must be higher than wait time in _tst_kill_test
+ sleep 15 # must be higher than wait time in _tst_kill_test
tst_res TFAIL "cleanup: running after TST_TIMEOUT"
}
diff --git a/lib/newlib_tests/shell/timeout04.sh
b/lib/newlib_tests/shell/timeout04.sh
index 0a6ba053c..c702905f3 100755
--- a/lib/newlib_tests/shell/timeout04.sh
+++ b/lib/newlib_tests/shell/timeout04.sh
@@ -9,7 +9,7 @@ TST_TIMEOUT=1
do_test()
{
- tst_res TINFO "Start"
+ tst_res TINFO "Start"
sleep 5
tst_res TFAIL "End"
}
diff --git a/testcases/lib/tst_test.sh b/testcases/lib/tst_test.sh
index 7ceddff04..ed2699175 100644
--- a/testcases/lib/tst_test.sh
+++ b/testcases/lib/tst_test.sh
@@ -469,7 +469,7 @@ _tst_timeout_process()
sleep $sec &
sleep_pid=$!
- #trap "kill $sleep_pid; exit" TERM
+ trap "kill $sleep_pid; exit" TERM
wait $sleep_pid
_tst_kill_test
}
================
(This below is not related to your patches)
But there is another issue I found that the timeout03 can NOT
be killed after timed out in calling cleanup(), the reason is
tst_brk stop the _tst_kill_test running in the background so that
it does not output as what we expected:
# timeout03 1 TINFO: timeout per run is 0h 0m 1s
# timeout03 1 TINFO: testing killing test after TST_TIMEOUT
# timeout03 1 TBROK: Test timeouted, sending SIGINT! If you are
running on slow machine, try exporting LTP_TIMEOUT_MUL > 1
# timeout03 1 TBROK: test interrupted or timed out
# timeout03 1 TPASS: test run cleanup after timeout
# timeout03 1 TINFO: Test is still running, waiting 10s
# timeout03 1 TINFO: Test is still running, waiting 9s
# timeout03 1 TINFO: Test is still running, waiting 8s
# timeout03 1 TINFO: Test is still running, waiting 7s
# timeout03 1 TINFO: Test is still running, waiting 6s
# timeout03 1 TINFO: Test is still running, waiting 5s
# timeout03 1 TINFO: Test is still running, waiting 4s
# timeout03 1 TINFO: Test is still running, waiting 3s
# timeout03 1 TINFO: Test is still running, waiting 2s
# timeout03 1 TINFO: Test is still running, waiting 1s
# timeout03 1 TBROK: Test still running, sending SIGKILL
# Killed
--
Regards,
Li Wang
Hi Li, Joerg, thanks both for your work! > +++ b/lib/newlib_tests/shell/timeout03.sh > @@ -32,7 +32,7 @@ do_test() > { > tst_res TINFO "testing killing test after TST_TIMEOUT" > - tst_sleep 2 > + sleep 2 +1 Although tst_test.sh has many LTP custom binary dependencies I agree to use simple sleep for seconds. > tst_res TFAIL "test: running after TST_TIMEOUT" > } > @@ -40,7 +40,7 @@ cleanup() > { > tst_res TPASS "test run cleanup after timeout" > - tst_sleep 15 # must be higher than wait time in _tst_kill_test > + sleep 15 # must be higher than wait time in _tst_kill_test and here. > tst_res TFAIL "cleanup: running after TST_TIMEOUT" > } > diff --git a/lib/newlib_tests/shell/timeout04.sh > b/lib/newlib_tests/shell/timeout04.sh > index 0a6ba053c..c702905f3 100755 > --- a/lib/newlib_tests/shell/timeout04.sh > +++ b/lib/newlib_tests/shell/timeout04.sh > @@ -9,7 +9,7 @@ TST_TIMEOUT=1 > do_test() > { > - tst_res TINFO "Start" > + tst_res TINFO "Start" > sleep 5 > tst_res TFAIL "End" > } > diff --git a/testcases/lib/tst_test.sh b/testcases/lib/tst_test.sh > index 7ceddff04..ed2699175 100644 > --- a/testcases/lib/tst_test.sh > +++ b/testcases/lib/tst_test.sh > @@ -469,7 +469,7 @@ _tst_timeout_process() > sleep $sec & > sleep_pid=$! > - #trap "kill $sleep_pid; exit" TERM > + trap "kill $sleep_pid; exit" TERM I guess trap use is needed, and commented out for testing. > wait $sleep_pid > _tst_kill_test > } > ================ > (This below is not related to your patches) > But there is another issue I found that the timeout03 can NOT > be killed after timed out in calling cleanup(), the reason is > tst_brk stop the _tst_kill_test running in the background so that > it does not output as what we expected: Good catch, I'll try to have a look as well. Kind regards, Petr > # timeout03 1 TINFO: timeout per run is 0h 0m 1s > # timeout03 1 TINFO: testing killing test after TST_TIMEOUT > # timeout03 1 TBROK: Test timeouted, sending SIGINT! If you are > running on slow machine, try exporting LTP_TIMEOUT_MUL > 1 > # timeout03 1 TBROK: test interrupted or timed out > # timeout03 1 TPASS: test run cleanup after timeout > # timeout03 1 TINFO: Test is still running, waiting 10s > # timeout03 1 TINFO: Test is still running, waiting 9s > # timeout03 1 TINFO: Test is still running, waiting 8s > # timeout03 1 TINFO: Test is still running, waiting 7s > # timeout03 1 TINFO: Test is still running, waiting 6s > # timeout03 1 TINFO: Test is still running, waiting 5s > # timeout03 1 TINFO: Test is still running, waiting 4s > # timeout03 1 TINFO: Test is still running, waiting 3s > # timeout03 1 TINFO: Test is still running, waiting 2s > # timeout03 1 TINFO: Test is still running, waiting 1s > # timeout03 1 TBROK: Test still running, sending SIGKILL > # Killed
> > ================ > > (This below is not related to your patches) > > > But there is another issue I found that the timeout03 can NOT > > be killed after timed out in calling cleanup(), the reason is > > tst_brk stop the _tst_kill_test running in the background so that > > it does not output as what we expected: > Good catch, I'll try to have a look as well. The simplest way I can think of is to let _tst_kill_test ignores the TERM and INT signals. If you agree to this, we can fix it in a separate patch:). --- a/testcases/lib/tst_test.sh +++ b/testcases/lib/tst_test.sh @@ -439,6 +439,8 @@ _tst_kill_test() { local i=10 + trap " " TERM INT + tst_res TBROK "Test timeouted, sending SIGINT! If you are running on slow machine, try exporting LTP_TIMEOUT_MUL > 1" kill -INT -$pid tst_sleep 100ms
Hi Li, > > > But there is another issue I found that the timeout03 can NOT > > > be killed after timed out in calling cleanup(), the reason is > > > tst_brk stop the _tst_kill_test running in the background so that > > > it does not output as what we expected: > > Good catch, I'll try to have a look as well. > The simplest way I can think of is to let _tst_kill_test ignores > the TERM and INT signals. If you agree to this, we can fix it in > a separate patch:). > --- a/testcases/lib/tst_test.sh > +++ b/testcases/lib/tst_test.sh > @@ -439,6 +439,8 @@ _tst_kill_test() > { > local i=10 > + trap " " TERM INT Acked-by: Petr Vorel <pvorel@suse.cz> Thanks! BTW I'm surprised " " works, I'd expect : would have to be used. Kind regards, Petr
> > --- a/testcases/lib/tst_test.sh > > +++ b/testcases/lib/tst_test.sh > > @@ -439,6 +439,8 @@ _tst_kill_test() > > { > > local i=10 > > > + trap " " TERM INT Seems no need to ignore the SIGTERM, because it only sends in the early (before timeout) phase, the _tst_kill_test has no chance to catch a SIGTERM. > Acked-by: Petr Vorel <pvorel@suse.cz> Thanks for the review! I will send a patch for it. > > Thanks! > > BTW I'm surprised " " works, I'd expect : would have to be used. " " or '' define none string makes it ignore signals.
diff --git a/testcases/lib/tst_test.sh b/testcases/lib/tst_test.sh index 951518785..7ceddff04 100644 --- a/testcases/lib/tst_test.sh +++ b/testcases/lib/tst_test.sh @@ -23,14 +23,6 @@ export TST_LIB_LOADED=1 # default trap function trap "tst_brk TBROK 'test interrupted or timed out'" INT -_tst_cleanup_timer() -{ - if [ -n "$_tst_setup_timer_pid" ]; then - kill $_tst_setup_timer_pid 2>/dev/null - wait $_tst_setup_timer_pid 2>/dev/null - fi -} - _tst_do_exit() { local ret=0 @@ -463,6 +455,25 @@ _tst_kill_test() fi } +_tst_cleanup_timer() +{ + if [ -n "$_tst_setup_timer_pid" ]; then + kill -TERM $_tst_setup_timer_pid 2>/dev/null + wait $_tst_setup_timer_pid 2>/dev/null + fi +} + +_tst_timeout_process() +{ + local sleep_pid + + sleep $sec & + sleep_pid=$! + #trap "kill $sleep_pid; exit" TERM + wait $sleep_pid + _tst_kill_test +} + _tst_setup_timer() { TST_TIMEOUT=${TST_TIMEOUT:-300} @@ -486,7 +497,8 @@ _tst_setup_timer() tst_res TINFO "timeout per run is ${h}h ${m}m ${s}s" _tst_cleanup_timer - sleep $sec && _tst_kill_test & + + _tst_timeout_process & _tst_setup_timer_pid=$! }