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.

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

Jump to bottom

Bugfix: Close all http connections before daemon shutdown. #38241

Closed
wants to merge 1 commit into from
Closed

Bugfix: Close all http connections before daemon shutdown. #38241

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
15 changes: 11 additions & 4 deletions api/server/server.go
View file
Open in desktop
Expand Up @@ -32,10 +32,11 @@ type Config struct {

// Server contains instance details for the server
type Server struct {
cfg *Config
servers []*HTTPServer
routers []router.Router
middlewares []middleware.Middleware
cfg *Config
servers []*HTTPServer
routers []router.Router
middlewares []middleware.Middleware
listenerClosed bool
}

// New returns a new instance of the server based on the specified configuration.
Expand Down Expand Up @@ -67,6 +68,7 @@ func (s *Server) Accept(addr string, listeners ...net.Listener) {

// Close closes servers and thus stop receiving requests
func (s *Server) Close() {
s.listenerClosed = true
for _, srv := range s.servers {
if err := srv.Close(); err != nil {
logrus.Error(err)
Expand Down Expand Up @@ -119,6 +121,11 @@ func (s *HTTPServer) Close() error {

func (s *Server) makeHTTPHandler(handler httputils.APIFunc) http.HandlerFunc {
return func(w http.ResponseWriter, r *http.Request) {
// Daemon is about to shutdown, server can't do api requets any more.
if s.listenerClosed {
Copy link
Member

Choose a reason for hiding this comment

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

We can't do this because this is async. This could use atomic or a mutex ... but really is it worth it?
Maybe instead we can inherit a context which is cancelled on shutdown?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cpuguy83 I'm not sure if I have got what you means,PTAL , thanks.

Copy link
Member

Choose a reason for hiding this comment

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

I think in order to get the desired result, we'll need to refactor how the server is started... namely we need to pass in a context that is tied to the daemon lifecycle so that it can be propagated to the request context.

return
}

// Define the context that we'll pass around to share info
// like the docker-request-id.
//
Expand Down