Skip to content

feat: graceful shutdown #1963

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.

Sign up for GitHub

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

Merged
sagikazarmark merged 3 commits into from
Feb 5, 2021
Merged

feat: graceful shutdown #1963

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f82c217
feat: graceful shutdown
nabokihms Jan 26, 2021
65a8bf2
feat: graceful shutdown fixes
nabokihms Jan 26, 2021
6664b57
Apply suggestions from code review
nabokihms Feb 5, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
113 changes: 77 additions & 36 deletions cmd/dex/serve.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,14 @@ import (
"net"
"net/http"
"os"
"os/signal"
"strings"
"syscall"
"time"

"github.com/ghodss/yaml"
grpcprometheus "github.com/grpc-ecosystem/go-grpc-prometheus"
"github.com/oklog/run"
"github.com/prometheus/client_golang/prometheus"
"github.com/prometheus/client_golang/prometheus/promhttp"
"github.com/sirupsen/logrus"
Expand Down Expand Up @@ -68,6 +71,28 @@ func commandServe() *cobra.Command {
return cmd
}

func listenAndShutdownGracefully(logger log.Logger, gr *run.Group, srv *http.Server, name string) error {
l, err := net.Listen("tcp", srv.Addr)
if err != nil {
return fmt.Errorf("listening (%s) on %s: %v", name, srv.Addr, err)
}

gr.Add(func() error {
logger.Infof("listening (%s) on %s", name, srv.Addr)
return srv.Serve(l)
}, func(err error) {
logger.Debugf("starting gracefully shutdown (%s)", name)
if err := l.Close(); err != nil {
logger.Errorf("gracefully close (%s) listener: %v", name, err)
}
Copy link
Member

@sagikazarmark sagikazarmark Jan 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIK the http server takes care of this, so you don't need to call close on the listener manually.

Copy link
Member Author

@nabokihms nabokihms Jan 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My thought was about splitting listener closing error and server shutdown error. Maybe it is excessive.

Copy link
Member Author

@nabokihms nabokihms Jan 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Listener closing now is a duty of a server.


if err := srv.Shutdown(context.Background()); err != nil {
Copy link
Member

@sagikazarmark sagikazarmark Jan 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if there is an active connection for an indefinite amount of time? Will this wait indefinitely? If so, we should probably add a timeout here.

Copy link
Member Author

@nabokihms nabokihms Jan 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, good catch. I hardcoded a one-minute timeout here. Please tell me if you want to make this configurable or increase the value.

logger.Errorf("gracefully shutdown (%s): %v", name, err)
}
})
return nil
}

func runServe(options serveOptions) error {
configFile := options.config
configData, err := ioutil.ReadFile(configFile)
Expand Down Expand Up @@ -302,21 +327,21 @@ func runServe(options serveOptions) error {
telemetryServ := http.NewServeMux()
telemetryServ.Handle("/metrics", promhttp.HandlerFor(prometheusRegistry, promhttp.HandlerOpts{}))

errc := make(chan error, 3)
var gr run.Group
if c.Telemetry.HTTP != "" {
logger.Infof("listening (http/telemetry) on %s", c.Telemetry.HTTP)
go func() {
err := http.ListenAndServe(c.Telemetry.HTTP, telemetryServ)
errc <- fmt.Errorf("listening on %s failed: %v", c.Telemetry.HTTP, err)
}()
telemetrySrv := &http.Server{Addr: c.Telemetry.HTTP, Handler: telemetryServ}
Copy link
Member

@sagikazarmark sagikazarmark Jan 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think defer httpServer.Close() should be called on each server in the main func as a general cleanup step and as a last resort to graceful shutdown.

if err := listenAndShutdownGracefully(logger, &gr, telemetrySrv, "http/telemetry"); err != nil {
return err
}
}

if c.Web.HTTP != "" {
logger.Infof("listening (http) on %s", c.Web.HTTP)
go func() {
err := http.ListenAndServe(c.Web.HTTP, serv)
errc <- fmt.Errorf("listening on %s failed: %v", c.Web.HTTP, err)
}()
httpSrv := &http.Server{Addr: c.Web.HTTP, Handler: serv}
if err := listenAndShutdownGracefully(logger, &gr, httpSrv, "http"); err != nil {
return err
}
}

if c.Web.HTTPS != "" {
httpsSrv := &http.Server{
Addr: c.Web.HTTPS,
Expand All @@ -327,35 +352,51 @@ func runServe(options serveOptions) error {
MinVersion: tls.VersionTLS12,
},
}

logger.Infof("listening (https) on %s", c.Web.HTTPS)
go func() {
err = httpsSrv.ListenAndServeTLS(c.Web.TLSCert, c.Web.TLSKey)
errc <- fmt.Errorf("listening on %s failed: %v", c.Web.HTTPS, err)
}()
if err := listenAndShutdownGracefully(logger, &gr, httpsSrv, "https"); err != nil {
return err
}
}

if c.GRPC.Addr != "" {
logger.Infof("listening (grpc) on %s", c.GRPC.Addr)
go func() {
errc <- func() error {
list, err := net.Listen("tcp", c.GRPC.Addr)
if err != nil {
return fmt.Errorf("listening on %s failed: %v", c.GRPC.Addr, err)
}
s := grpc.NewServer(grpcOptions...)
api.RegisterDexServer(s, server.NewAPI(serverConfig.Storage, logger))
grpcMetrics.InitializeMetrics(s)
if c.GRPC.Reflection {
logger.Info("enabling reflection in grpc service")
reflection.Register(s)
}
err = s.Serve(list)
return fmt.Errorf("listening on %s failed: %v", c.GRPC.Addr, err)
}()
}()
grpcListener, err := net.Listen("tcp", c.GRPC.Addr)
if err != nil {
return fmt.Errorf("listening (grcp) on %s: %w", c.GRPC.Addr, err)
}

grpcSrv := grpc.NewServer(grpcOptions...)
api.RegisterDexServer(grpcSrv, server.NewAPI(serverConfig.Storage, logger))
grpcMetrics.InitializeMetrics(grpcSrv)
if c.GRPC.Reflection {
logger.Info("enabling reflection in grpc service")
reflection.Register(grpcSrv)
}

gr.Add(func() error {
logger.Infof("listening (grpc) on %s", c.GRPC.Addr)
return grpcSrv.Serve(grpcListener)
}, func(err error) {
logger.Debugf("starting gracefully shutdown (grpc)")
if err := grpcListener.Close(); err != nil {
logger.Errorf("failed to gracefully close (grpc) listener: %v", err)
}
Copy link
Member

@sagikazarmark sagikazarmark Jan 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above

grpcSrv.GracefulStop()
})
}

return <-errc
sig := make(chan os.Signal)
gr.Add(func() error {
Copy link
Member

@sagikazarmark sagikazarmark Jan 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The library has a builtin signal actor, maybe we could use it.

Copy link
Member Author

@nabokihms nabokihms Jan 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. The only thing that I dislike about helper is that I can't log the received signal immediately.

signal.Notify(sig, os.Interrupt, syscall.SIGTERM)
receivedSig := <-sig
logger.Infof("received %s signal, shutting down", receivedSig)
return nil
}, func(err error) {
close(sig)
})

if err := gr.Run(); err != nil {
return fmt.Errorf("run groups: %w", err)
}
return nil
}

var (
Expand Down
1 change: 1 addition & 0 deletions go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ require (
github.com/lib/pq v1.9.0
github.com/mattermost/xml-roundtrip-validator v0.0.0-20201219040909-8fd2afad43d1
github.com/mattn/go-sqlite3 v1.14.6
github.com/oklog/run v1.1.0
github.com/pkg/errors v0.9.1
github.com/prometheus/client_golang v1.4.0
github.com/russellhaering/goxmldsig v1.1.0
Expand Down
2 changes: 2 additions & 0 deletions go.sum
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,6 +253,8 @@ github.com/modern-go/reflect2 v1.0.1/go.mod h1:bx2lNnkwVCuqBIxFjflWJWanXIb3Rllmb
github.com/morikuni/aec v0.0.0-20170113033406-39771216ff4c h1:nXxl5PrvVm2L/wCy8dQu6DMTwH4oIuGN8GJDAlqDdVE=
github.com/morikuni/aec v0.0.0-20170113033406-39771216ff4c/go.mod h1:BbKIizmSmc5MMPqRYbxO4ZU0S0+P200+tUnFx7PXmsc=
github.com/mwitkow/go-conntrack v0.0.0-20161129095857-cc309e4a2223/go.mod h1:qRWi+5nqEBWmkhHvq77mSJWrCKwh8bxhgT7d/eI7P4U=
github.com/oklog/run v1.1.0 h1:GEenZ1cK0+q0+wsJew9qUg/DyD8k3JzYsZAi5gYi2mA=
github.com/oklog/run v1.1.0/go.mod h1:sVPdnTZT1zYwAJeCMu2Th4T21pA3FPOQRfWjQlk7DVU=
github.com/oklog/ulid v1.3.1/go.mod h1:CirwcVhetQ6Lv90oh/F+FBtV6XMibvdAFo93nm5qn4U=
github.com/olekukonko/tablewriter v0.0.0-20170122224234-a0225b3f23b5/go.mod h1:vsDQFd/mU46D+Z4whnwzcISnGGzXWMclvtLoiIKAKIo=
github.com/onsi/ginkgo v1.4.0/go.mod h1:lLunBs/Ym6LB5Z9jYTR76FiuTmxDTDusOGeTQH+WWjE=
Expand Down