add Wait() for nslistener process to avoid zombies

[pmorjan]

This patch adds a missing cmd.Wait() to wait for process completion
of containerd-nslistener. Without the wait every container leaves
a defunct process. This is required since #430 removed the global
SIGCHILD handler to avoid runtime panics.

Signed-off-by: Peter Morjan peter.morjan@de.ibm.com

[laijs]

It seems strange that Process.Signal() and Process.Kill() are called at the same time.
if Process.Kill() is required when Process.Signal() is ignored by the target, I think a timer is also needed before Process.Kill().

[laijs]

I think cmd.Wait() should be called unconditionally, WDYT?

[pmorjan]

The signal(0) was just to ensure the process is really alive to prevent runtime panic on cmd.Wait.
Do you prefer it like this?

-               hp.nslistener.cmd.Process.Kill()
+               go func() {
+                       time.Sleep(time.Second*1)
+                       hp.nslistener.cmd.Process.Kill()
+               }()
+               hp.nslistener.cmd.Wait()

[laijs]

use time.AfterFunc()

"if hp.nslistener != nil" ensures the process is/was launched

[laijs]

I like something like this

                hp.nslistener.cmd.Process.Signal(syscall.SIGTERM)
                timer := time.AfterFunc(time.Second*1, func() { hp.nslistener.cmd.Process.Kill() })
                hp.nslistener.cmd.Wait()
                timer.Stop()

or just kill&wait, no timer&signal

[pmorjan]

i don't get the difference. Please fix it as you like. Thanks.

[laijs]

there is no Signal() in your code, so it will always take 1 seconds to stop the nslistener

[laijs]

@pmorjan runv cli is being improved continuously and had just been refactored #537 . As a result, it seams that the changes in this pr is outdated.

In the new code, this problem is fixed by this code:
https://github.com/hyperhq/runv/blob/master/cli/network.go#L320-L325

	defer func() {
		if err != nil {
			cmd.Process.Kill()
		}
		cmd.Wait()
	}()

Thank you for your contribution.