thejerf / suture

Supervisor trees for Go.
MIT License
1.32k stars 74 forks source link

Racy panic when calling Supervisor.Stop directly after Supervisor.ServeBackground #35

Closed imsodin closed 5 years ago

imsodin commented 5 years ago

Since v3.0.1 a test in Syncthing panics with

Panic: Stop() called on not-currently-running supervisor

goroutine 27 [running]:
github.com/thejerf/suture.(*Supervisor).Stop(0xc0001a40f0)
    /media/ext4_data/Coding/go/pkg/mod/github.com/thejerf/suture@v3.0.1+incompatible/supervisor.go:532 +0x108
github.com/thejerf/suture.(*Supervisor).stopSupervisor.func1(0x7fa9a1546e70, 0xc000144600, 0x93abbe, 0xf, 0xc000186070, 0x0)
    /media/ext4_data/Coding/go/pkg/mod/github.com/thejerf/suture@v3.0.1+incompatible/supervisor.go:651 +0x31
created by github.com/thejerf/suture.(*Supervisor).stopSupervisor
    /media/ext4_data/Coding/go/pkg/mod/github.com/thejerf/suture@v3.0.1+incompatible/supervisor.go:650 +0x204
exit status 2

The following code reproduces the problem (you may need to adjust the amount of loops for it to trigger):

package main

import "github.com/thejerf/suture"

type sup struct {
    *suture.Supervisor
}

func newSup() *sup {
    return &sup{
        Supervisor: suture.New("local", suture.Spec{
            PassThroughPanics: true,
        }),
    }
}

func main() {
    s := newSup()
    os := s
    var ns *sup
    for i := 0; i < 100; i++ {
        ns = newSup()
        os.Add(ns)
        os = ns
    }

    s.ServeBackground()
    s.Stop()
}
thejerf commented 5 years ago

Thank you for catching that. In hindsight, it's obvious that I did the wrong thing in 3.0.1, but it should be corrected now.

I've added that test to my test suite as well.