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

Ability to shutdown server with error #156

Open
ryanmcnamara opened this issue Jan 21, 2020 · 3 comments
Open

Ability to shutdown server with error #156

ryanmcnamara opened this issue Jan 21, 2020 · 3 comments

Comments

@ryanmcnamara
Copy link

ryanmcnamara commented Jan 21, 2020

Feature Request

A common pattern is to run background tasks (goroutines) from an init server function (e.g. a controller). When these background tasks fail, (consider the case where due to a programmer bug there is a panic)

It would be nice to write an init func something like

func main() {
    err := startServer()
    if err != nil {
        os.Exit(1)
    }
    os.Exit(0)
}

func startServer() error {
	return witchcraft.NewServer().
		WithInitFunc(initFunc).
		Start()
}

func initFunc(ctx context.Context, info witchcraft.InitInfo) (cleanup func(), rErr error) {
	go func() {
        err := doBackgroundTask(ctx)

        // ... log / handle error

		if err != nil{
			err := info.ShutdownServerWithError(ctx, err)

            // ... handle err

		} else {
            info.ShutdownServer(ctx)
        }
	}()
	return nil, doRun(ctx,conf)
}

And have server.Start() actually return an error as desired.

Strawman proposal would be to add a new method to InitInfo:

	// ShutdownServerWithErr closes the server, waiting for any in-flight requests to finish (or the context to be cancelled).
	// The server is shutdown with an error, causing `Start` to return an error.
	ShutdownServer func(context.Context, err error) error

I'd be happy to implement this if this sounds good.

@nmiyake
Copy link
Contributor

nmiyake commented Jan 21, 2020

Thanks for the feedback. Just for clarification, is it the case that the current Close() and Shutdown(context.Context) functions do close/shut down the server, but cause the start function to just return nil rather than an error?

@ryanmcnamara
Copy link
Author

Yup, exactly

@nmiyake
Copy link
Contributor

nmiyake commented Jan 28, 2020

Got it. Yes, I think this is reasonable -- would just say that, in the comment/documentation for the function, you should note that the function is the equivalent of calling Shutdown, but will return a non-nil error. I think it would also make sense for the PR to update the implementation of Shutdown to use ShutdownServer with a nil error.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants