[ovs-dev] [PATCH v2] tests: Avoid Windows unit tests from hanging.

[Gurucharan Shetty]

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
because the cleanup file tries to run the command
"kill `cat ovs-vswitchd.pid`" and ovs-vswitchd.pid no longer exists.

With this commit, we write the pid value of the daemons in the
cleanup file (instead of asking it to 'cat' the value later from
the pidfile). This way, even if the pidfiles get deleted, 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>
(ON_EXIT_UNQUOTED macro provided by Ben.)
Co-authored-by: Ben Pfaff <blp at nicira.com>
---
 tests/ofproto-macros.at |    3 ++-
 tests/ovs-macros.at     |   35 ++++++++++++++++++++++++++---------
 2 files changed, 28 insertions(+), 10 deletions(-)

diff --git a/tests/ofproto-macros.at b/tests/ofproto-macros.at
index 93d8a77..fd915ef 100644
--- a/tests/ofproto-macros.at
+++ b/tests/ofproto-macros.at
@@ -49,7 +49,6 @@ m4_define([_OVS_VSWITCHD_START],
    OVS_LOGDIR=`pwd`; export OVS_LOGDIR
    OVS_DBDIR=`pwd`; export OVS_DBDIR
    OVS_SYSCONFDIR=`pwd`; export OVS_SYSCONFDIR
-   ON_EXIT([kill `cat ovsdb-server.pid ovs-vswitchd.pid`])
 
    dnl Create database.
    touch .conf.db.~lock~
@@ -57,6 +56,7 @@ m4_define([_OVS_VSWITCHD_START],
 
    dnl Start ovsdb-server.
    AT_CHECK([ovsdb-server --detach --no-chdir --pidfile --log-file --remote=punix:$OVS_RUNDIR/db.sock], [0], [], [stderr])
+    ON_EXIT_UNQUOTED([kill `cat ovsdb-server.pid`])
    AT_CHECK([[sed < stderr '
 /vlog|INFO|opened log file/d
 /ovsdb_server|INFO|ovsdb-server (Open vSwitch)/d']])
@@ -68,6 +68,7 @@ m4_define([_OVS_VSWITCHD_START],
    dnl Start ovs-vswitchd.
    AT_CHECK([ovs-vswitchd $1 --detach --no-chdir --pidfile --log-file -vvconn -vofproto_dpif], [0], [], [stderr])
    AT_CAPTURE_FILE([ovs-vswitchd.log])
+   ON_EXIT_UNQUOTED([kill `cat ovs-vswitchd.pid`])
    AT_CHECK([[sed < stderr '
 /ovs_numa|INFO|Discovered /d
 /vlog|INFO|opened log file/d
diff --git a/tests/ovs-macros.at b/tests/ovs-macros.at
index 14edba3..c583c3d 100644
--- a/tests/ovs-macros.at
+++ b/tests/ovs-macros.at
@@ -55,12 +55,16 @@ if test "$IS_WIN32" = "yes"; then
             -[1-9]*)
                 shift
                 for i in $*; do
-                    taskkill //F //PID $i >/dev/null
+                    if tasklist //fi "PID eq $i" | grep $i >/dev/null; then
+                        tskill $i
+                    fi
                 done
                 ;;
             [1-9][0-9]*)
                 for i in $*; do
-                    taskkill //F //PID $i >/dev/null
+                    if tasklist //fi "PID eq $i" | grep $i >/dev/null; then
+                        tskill $i
+                    fi
                 done
                 ;;
         esac
@@ -86,15 +90,28 @@ 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])])
-- 
1.7.9.5