New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

"set -e" causes atf-sh subshells to exit prematurely #14

Closed
asomers opened this Issue Sep 4, 2014 · 3 comments

Comments

Projects
None yet
2 participants
@asomers
Copy link

asomers commented Sep 4, 2014

commit 7d68b8a added "set -e" to libatf-sh.subr. I don't know what original problem it tried to fix, because atf seems to have been using a different bug tracker back then. But "set -e" has unexpected consequences. Among them, backgrounded subshells whose commands are considered to be unchecked by /bin/sh will terminate as soon as one of those commands exits nonzero. You don't notice the effect for foregrounded subshells because they are wrapped in a function (the ATF test case body) and that function's exit status is checked by libatf-sh.subr.

Simply disabling "set -e" does not cause any of atf's builtin testcases to fail. Probably, this is because the truly important changes in commit 7d68b8a were checking the exit status of ${_tcname}_body and ${_tcname}_cleanup.

The problem can be demonstrated by the atf test program below. Were it not for "set -e", all four of the background and foreground tests would timeout. But due to "set -e", the loop in background_false exits early and that test case passes.

#! /usr/libexec/atf-sh

atf_test_case background_false
background_false_head()
{
    atf_set "timeout" "1"
}
background_false_body()
{
    while true; do
        false
    done &
    wait $!
    echo "background loop exited $?"
}

atf_test_case background_true
background_true_head()
{
    atf_set "timeout" "1"
}
background_true_body()
{
    while true; do
        true
    done &
    wait $!
    echo "background loop exited $?"
}

atf_test_case foreground_false
foreground_false_head()
{
    atf_set "timeout" "1"
}
foreground_false_body()
{
    while true; do
        false
    done
    echo "foreground loop exited $?"
}

atf_test_case foreground_true
foreground_true_head()
{
    atf_set "timeout" "1"
}
foreground_true_body()
{
    while true; do
        true
    done
    echo "foreground loop exited $?"
}

atf_test_case nonloop
nonloop_head()
{
    atf_set "timeout" "1"
}
nonloop_body()
{
    false
    echo "false command exited $?"
}

atf_init_test_cases()
{
    atf_add_test_case background_false
    atf_add_test_case background_true
    atf_add_test_case foreground_false
    atf_add_test_case foreground_true
    atf_add_test_case nonloop
}

@jmmv jmmv added the bug label Sep 8, 2014

@jmmv

This comment has been minimized.

Copy link
Owner

jmmv commented Oct 5, 2014

The original motivation for set -e had nothing to do with fixing internal test failures. The motivation was to make the code of test cases more strict by preventing unchecked commands from failing in a silent manner (and, as a side-effect, that exposed some breakage).

The test cases you post above are "broken" in a sense because they do nothing with the output value captured by wait. Just printing the value is not sufficient for executing a test. I'd argue that if you background a command, it's your responsibility to properly check its exit code.

I could agree that atf-sh maybe should not be in the business of enabling set -e and the caller test programs should explicitly do so if they choose to. Or vice-versa: we could document in atf-sh-api(3) that atf-sh enables set -e and that test programs should do set +e at startup if they don't want that behavior.

What problem are you trying to fix? Is it just unexpected behavior due to the lack of documentation? Or do you see a different technical reason for not enabling set -e by default?

@asomers

This comment has been minimized.

Copy link
Author

asomers commented Oct 6, 2014

The biggest problem is an inconsistency: backgrounded subshells require all commands to be checked, but foregrounded subshells do not. That's a serious POLA violation. Also, it's just not what shell programmers expect. Here's a link to several weird cases caused by "set -e" that are hard to understand. Even though it's written about bash, it mostly applies to sh as well.

http://mywiki.wooledge.org/BashFAQ/105

Even the FreeBSD sh(1) man page recommends against using "set -e"
https://www.freebsd.org/cgi/man.cgi?query=sh&apropos=0&sektion=0&manpath=FreeBSD+11-current&arch=default&format=html

@jmmv

This comment has been minimized.

Copy link
Owner

jmmv commented Oct 9, 2014

Fair enough. I'll remove this from atf-sh but will add a test to ensure that atf-sh can be used with or without set -e, should any developer out there want to enable the setting in their code.

@jmmv jmmv closed this in 35b7485 Oct 9, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment