Repository: kubernetes
Issue: pkg/util/interrupt/interrupt.go has big bad bugs
Labels: kind/bug,sig/architecture,lifecycle/frozen

Discovered in the context of https://github.com/kubernetes/utils/pull/87, I will duplicate some of the thoughts here.  If someone takes this, please go read that PR (unless it gets fixed up and merged, in which case close this)

https://github.com/kubernetes/utils/pull/87#pullrequestreview-220631325

1) Is it allowed to call Chain() and then Run() from within a higher-level Run() ?

```
h1 := New(myfinal, foo, bar)
outer := func() error {
    h2 := Chain(h1, bat, baz)
    h2.Run(inner)
    // other stuff
    return nil
}
h1.Run(outer)
```

So outer() starts, Chains h2, and runs inner().  When inner() completes, this calls {bat, baz, foo, bar} in that order, right?  Then, when outer() completes, it *WILL NOT* run foo and bar, because the `once` was already triggered.

That seems very wrong.  If I understand correctly, we should at least explicitly document this.  Something like:

Calling Chain on a handler which is already being run, either from within the critical section or externally, will produce undefined behavior and is almost certainly NOT what you want.

In fact, maybe we should specifically track this in `running bool` (guarded by a mutex) and have Chain() return nil (or even panic() ?) if called on a running handler?

Calling Run() on a Handler twice could also be returned as an error rather than just not working.

Calling Run() on a child and a parent in serial is also wrong, and we should defend against it.

Here's a fully formed demonstration:

```
package main

import (
    "fmt"
    "os"
    "syscall"
    "time"
)

func main() {
    sig()
    nestClean()
    nestSignal()
}
                                                                                                                            
func sig() {
    fmt.Println("TEST SIGNAL")
    fmt.Println("open foo")
    fmt.Println("open bar")
    h1 := New(myfinal, foo, bar)
    h1.Run(func() error {
        raise(syscall.SIGTERM)
        time.Sleep(100 * time.Millisecond)
        return nil 
    })  
    fmt.Println("")
}
func nestClean() {
    fmt.Println("TEST NESTED w/o signal")
    fmt.Println("open foo")
    fmt.Println("open bar")
    h1 := New(myfinal, foo, bar)
    h1.Run(func() error {
        fmt.Println("open qux")
        h2 := Chain(h1, qux)
        return h2.Run(func() error { return nil })
    })  
    fmt.Println("")
}

func nestSignal() {
    fmt.Println("TEST NESTED w/ signal")
    fmt.Println("open foo")
    fmt.Println("open bar")
    h1 := New(myfinal, foo, bar)
    h1.Run(func() error {
        fmt.Println("open qux")
        h2 := Chain(h1, qux)
        return h2.Run(func() error { return raise(syscall.SIGTERM) })
    })  
    fmt.Println("")
}

func myfinal(s os.Signal) {
    fmt.Printf("caught signal %q\n", s)
}
func foo() {
    fmt.Println("close foo")
}
func bar() {
    fmt.Println("close bar")
}
func qux() {
    fmt.Println("close qux")
}

func raise(sig os.Signal) error {
    p, err := os.FindProcess(os.Getpid())
    if err != nil {
        return err
    }
    return p.Signal(sig)
}                                
```

consider:

```
h1 := New(myfinal, foo, bar)
h2 := Chain(h1, bat, qux)
h2.Run(something)
```

In the case of normal completion,  we call `h2.close()` which calls `bat()` and `qux()`, then `h1.close()` which calls `foo()` and `bar()`.  OK

In the case of a signal being delivered, we call `h2.signal()` which calls `bat()` and `qux()`, then `h1.close()` which calls `foo()` and `bar()`.  Then it calls `h1.signal()` which *DOES NOTHING* because the `once` has already been triggered.  So `myfinal() is never called`.

I think you need to pass `handler.final` not `handler.signal` ?

Additionally this does not cleanly handle sub-sections - is it supposed to ?  See trouble example at Run() below.

This whole Chain() API feels fraught to me.  Because of the `once`, calling Chain() is effectively mutating (or invalidating) the original handler.  Can we make that more obvious in the API?

Either say that Chain() "copies" the parent handler's contents (and do `append(notify, handler.notify...)`) or say that Chain() takes modifies the original handler (e.g. (`h := New(myfinal, foo, bar); h.Chain(bat, baz); h.Run()` or something.

A fully formed example.  Note that myfinal is never called in the chain case

```
package main

import (
    "fmt"
    "os"
    "syscall"
    "time"
)

func main() {
    sig()
    chain()
}

func sig() {
    fmt.Println("TEST SIGNAL")
    fmt.Println("open foo")
    fmt.Println("open bar")
    h1 := New(myfinal, foo, bar)
    h1.Run(func() error {
        raise(syscall.SIGTERM)
        time.Sleep(100 * time.Millisecond)
        return nil 
    })  
    fmt.Println("")
}

func chain() {
    fmt.Println("TEST CHAIN SIGNAL")
    fmt.Println("open foo")
    fmt.Println("open bar")
    h1 := New(myfinal, foo, bar)
    fmt.Println("open qux")                                                                                                 
    h2 := Chain(h1, qux)
    h2.Run(func() error {
        raise(syscall.SIGTERM)
        time.Sleep(100 * time.Millisecond)
        return nil 
    })  
    fmt.Println("")
}
func myfinal(s os.Signal) {
    fmt.Printf("caught signal %q\n", s)
}
func foo() {
    fmt.Println("close foo")
}
func bar() {
    fmt.Println("close bar")
}
func qux() {
    fmt.Println("close qux")
}

func raise(sig os.Signal) error {
    p, err := os.FindProcess(os.Getpid())
    if err != nil {
        return err
    }
    return p.Signal(sig)
}                                  
```