uber-go / fx

A dependency injection based application framework for Go.
https://uber-go.github.io/fx/
MIT License
5.48k stars 283 forks source link

Deadlock in `signalReceivers.Stop` #1219

Open ogaca-dd opened 2 weeks ago

ogaca-dd commented 2 weeks ago

Describe the bug There is a deadlock in signalReceivers.Stop:

There is a deadlock.

To Reproduce Run the following code:

func main() {
    fx.New(fx.Invoke(func(shutdowner fx.Shutdowner) {
        go func() {
            _ = syscall.Kill(syscall.Getpid(), syscall.SIGINT)
            _ = shutdowner.Shutdown()
        }()
    })).Run()
}

Expected behavior No deadlock

JacobOaks commented 2 weeks ago

Hey @ogaca-dd thanks for the report, let me look into this.

Can I ask what kinds of situations you're experiencing where a shutdown signal and an OS signal are being sent at roughly the same time?

ogaca-dd commented 2 weeks ago

@JacobOaks Thanks for your quick answer.

In the situation described above, I am experiencing a dead lock meaning the application try to stop but freeze forever.

JacobOaks commented 2 weeks ago

Hey @ogaca-dd - I understand. My question is why would an application call shutdowner.Shutdown() after it gets killed? The example you provide seems pretty contrived so I'm just trying to better understand the real-world impact here.

ogaca-dd commented 2 weeks ago

The following code is the simplest code I found to reproduce the issue.

func main() {
    fx.New(fx.Invoke(func(shutdowner fx.Shutdowner) {
        go func() {
            _ = syscall.Kill(syscall.Getpid(), syscall.SIGINT)
            _ = shutdowner.Shutdown()
        }()
    })).Run()
}

Our real code looks like

        sigChan := make(chan os.Signal, 1)
    signal.Notify(sigChan, syscall.SIGINT, syscall.SIGTERM, syscall.SIGPIPE)
    for signo := range sigChan {
        switch signo {
        case syscall.SIGINT, syscall.SIGTERM:
            log.Infof("Received signal %d (%v)", signo, signo)
             shutdowner.Shutdown()
            return
                }
               // Some code here
       }
}

I know that fx already handles signals and this code should be written differently (This code was not updated during our migration to fx).

Anyway, the code I pointed contains a dead lock which was tricky to find. In our case, this freeze happens randomly:

I understand it may not be a correct usage of fx but as a user I don't expect to have my application freeze randomly in a such case.

JacobOaks commented 2 weeks ago

Gotcha. Yeah I was able to reproduce this locally and I will try to figure out the best way to update the signal handlers to prevent this, but yes - as you mentioned - using app.Run will cause Fx to respond to signals automatically. So if you want to workaround this for now you can either:

ogaca-dd commented 2 weeks ago

Thanks for your quick reply and for the workarounds.