[ovs-dev] [PATCH 1/2] tests: Avoid Windows unit tests from hanging.

[Gurucharan Shetty]

On Tue, Apr 14, 2015 at 9:02 PM, Ben Pfaff <blp at nicira.com> wrote:
> On Tue, Apr 07, 2015 at 06:06:40PM -0700, Gurucharan Shetty wrote:
>> It has been observed that sometimes Windows unit tests hang.
>> This happens when a daemon is started but does not get terminated
>> when the test ends.
>>
>> In one particular case, OVS_VSWITCHD_STOP is called which inturn
>> calls 'ovs-appctl exit'. This causes ovs-vswitchd's atexit handler
>> to cleanup the pidfiles. After this, the pthread destructurs get
>> called and a deadlock happens in there. This results in the
>> daemons not getting force killed resulting in the tests hanging.
>>
>> This commit stores the contents of pidfiles in different files.
>> This way, even if the pidfiles get deleted for any reason, we
>> can still kill the daemons.
>>
>> This commit also changes the way daemons are force killed in
>> Windows. It was observed that 'taskkill //F ' failed to kill
>> a deadlocked daemon running its pthread destructor. But
>> tskill succeeds.
>>
>> Signed-off-by: Gurucharan Shetty <gshetty at nicira.com>
>
> This seems reasonable to me.
>
> I think that we might be able to implement it a little more nicely.
> What if we introduce an ON_EXIT_UNQUOTED, analogous to
> AT_CHECK_UNQUOTED, and use it here in OVS_VSWITCHD_START instead of
> making a on-disk file copy?

I am sorry for the delayed response. I somehow completely missed your feedback.
The change that you suggest is very clever (I did not know the
difference between quoted and unquoted word after the HERE document
till today.) . I will incorporate it as part of v2.


>
> diff --git a/tests/ovs-macros.at b/tests/ovs-macros.at
> index 14edba3..d06e200 100644
> --- a/tests/ovs-macros.at
> +++ b/tests/ovs-macros.at
> @@ -86,15 +86,29 @@ m4_define([OVS_APP_EXIT_AND_WAIT],
>    [ovs-appctl -t $1 exit
>     OVS_WAIT_WHILE([test -e $1.pid])])
>
> +m4_define([ON_EXIT__], [trap '. ./cleanup' 0; cat - cleanup << $2 > __cleanup
> +$1
> +EOF
> +mv __cleanup cleanup
> +])
> +
>  dnl ON_EXIT([COMMANDS])
> +dnl ON_EXIT_UNQUOTED([COMMANDS])
>  dnl
> -dnl Adds the shell COMMANDS to a collection executed when the current test
> +dnl Add the shell COMMANDS to a collection executed when the current test
>  dnl completes, as a cleanup action.  (The most common use is to kill a
>  dnl daemon started by the test.  This is important to prevent tests that
>  dnl start daemons from hanging at exit.)
> -dnl The commands will be added will be tht first one to excute.
> -m4_define([ON_EXIT], [trap '. ./cleanup' 0; cat - cleanup << 'EOF' > __cleanup
> -$1
> -EOF
> -mv __cleanup cleanup
> -])
> +dnl
> +dnl The only difference between ON_EXIT and ON_EXIT_UNQUOTED is that only the
> +dnl latter performs shell variable (e.g. $var) substitution, command
> +dnl substitution (e.g. `command`), and backslash escaping (e.g. \\ becomes \)
> +dnl in COMMANDS at the time that ON_EXIT_UNQUOTED is encountered.  ON_EXIT,
> +dnl in contrast, copies the literal COMMANDS and only executes shell expansion
> +dnl at cleanup time.
> +dnl
> +dnl Cleanup commands are executed in the reverse order of execution of
> +dnl these macros.
> +m4_define([ON_EXIT], [ON_EXIT([$1], ['EOF'])])
> +m4_define([ON_EXIT_UNQUOTED], [ON_EXIT([$1], [EOF])])
> +
>