Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

onStop executed many times #930

Closed
sywhang opened this issue Aug 23, 2022 Discussed in #929 · 1 comment · Fixed by #931
Closed

onStop executed many times #930

sywhang opened this issue Aug 23, 2022 Discussed in #929 · 1 comment · Fixed by #931
Assignees
Labels

Comments

@sywhang
Copy link
Contributor

sywhang commented Aug 23, 2022

Discussed in #929

Originally posted by coolyrat August 22, 2022

package main

import (
	"context"
	"fmt"
	"log"
	"os"
	"os/signal"
	"syscall"
	"time"

	"go.uber.org/fx"
)

func main() {
	server := fx.New(
		fx.Invoke(func(lc fx.Lifecycle) {
			fmt.Println("first invoke...")
			lc.Append(fx.Hook{
				OnStart: func(ctx context.Context) error {
					fmt.Println("#1 start")
					return nil
				},
				OnStop: func(ctx context.Context) error {
					fmt.Println("#1 stop")
					return nil
				},
			})
		}),

		fx.Invoke(func(lc fx.Lifecycle) {
			fmt.Println("last invoke...")
			lc.Append(fx.Hook{
				OnStart: func(ctx context.Context) error {
					fmt.Println("#2 start")
					return nil
				},
				OnStop: func(ctx context.Context) error {
					fmt.Println("#2 stop")
					return nil
				},
			})
		}),
	)

	go func() {
		server.Run()
	}()

	kill := make(chan os.Signal, 1)
	signal.Notify(kill, syscall.SIGINT, syscall.SIGTERM, syscall.SIGKILL, syscall.SIGQUIT)
	<-kill
	fmt.Println("killed")

	ctx, _ := context.WithTimeout(context.Background(), 10*time.Second)
	if err := server.Stop(ctx); err != nil {
		log.Printf("error stopping gracefully")
	}
}

When I give the stop signal, the log

...
killed
#2 stop
#1 stop
#2 stop

And I am expected to be

...
killed
#2 stop
#1 stop

Is there anything I'm doing wrong?

@sywhang sywhang changed the title onStop executed multi times onStop executed many times Aug 23, 2022
@sywhang sywhang added the bug label Aug 23, 2022
@sywhang sywhang self-assigned this Aug 23, 2022
@sywhang
Copy link
Contributor Author

sywhang commented Aug 23, 2022

Root-cause: the signal handler that gets registered on SIGINT races with the Stop() method being invoked by the user code.

sywhang added a commit to sywhang/fx that referenced this issue Aug 23, 2022
It is possible for a user to erroneously call app.Run() then
follow up by calling app.Stop(). In such a case, it is possible
for the app.Stop() method to be called by two goroutines concurrently,
resulting in a race.

This adds a state in the App to keep track of whether Stop() has been
invoked so that such a race can be prevented.

Fix uber-go#930
Internal Ref: GO-1606
sywhang added a commit that referenced this issue Aug 24, 2022
* Ensure OnStop hooks can only be called once

It is possible for a user to erroneously call app.Run() then
follow up by calling app.Stop(). In such a case, it is possible
for the app.Stop() method to be called by two goroutines concurrently,
resulting in a race.

This adds a state in the App to keep track of whether Stop() has been
invoked so that such a race can be prevented.

Fix #930
Internal Ref: GO-1606

* use sync.Once and also add the check for OnStart hooks

* Apply suggestions from code review

Co-authored-by: Abhinav Gupta <mail@abhinavg.net>

Co-authored-by: Abhinav Gupta <mail@abhinavg.net>
sywhang added a commit to sywhang/fx that referenced this issue Oct 11, 2022
* Ensure OnStop hooks can only be called once

It is possible for a user to erroneously call app.Run() then
follow up by calling app.Stop(). In such a case, it is possible
for the app.Stop() method to be called by two goroutines concurrently,
resulting in a race.

This adds a state in the App to keep track of whether Stop() has been
invoked so that such a race can be prevented.

Fix uber-go#930
Internal Ref: GO-1606

* use sync.Once and also add the check for OnStart hooks

* Apply suggestions from code review

Co-authored-by: Abhinav Gupta <mail@abhinavg.net>

Co-authored-by: Abhinav Gupta <mail@abhinavg.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging a pull request may close this issue.

1 participant