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

Jump to bottom

net/http: server's Shutdown() method is not graceful enough #32116

Open
chenjie199234 opened this issue May 17, 2019 · 9 comments
Open

net/http: server's Shutdown() method is not graceful enough #32116

chenjie199234 opened this issue May 17, 2019 · 9 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Unplanned

Comments

@chenjie199234
Copy link

Http server's registered func on shutdown is called in goroutine.
It will be interrupted by end of the process.

package main

import (
	"context"
	"fmt"
	"net/http"
	"os"
	"os/signal"
	"syscall"
	"time"
)

func main() {
	var closing bool
	m := http.DefaultServeMux
	m.HandleFunc("/", root)
	s := &http.Server{
		Addr:    ":8080",
		Handler: m,
	}
	s.RegisterOnShutdown(shutdown_func1)
	s.RegisterOnShutdown(shutdown_func2)
	sigs := make(chan os.Signal, 1)
	signal.Notify(sigs, syscall.SIGINT)
	done := make(chan bool, 1)
	go func() {
		fmt.Println("start server")
		s.ListenAndServe()
		fmt.Println("end server")
		done <- true
	}()
	for {
		select {
		case <-sigs:
			if !closing {
				fmt.Println("start shutting down")
				closing = true
				s.Shutdown(context.Background())
				fmt.Println("end shutting down")
			}
		case <-done:
			return
		}
	}
}
func root(w http.ResponseWriter, r *http.Request) {
	fmt.Println("root")
}
func shutdown_func1() {
	time.Sleep(time.Second)
	fmt.Println("shutdown call back 1")
}
func shutdown_func2() {
	time.Sleep(time.Second)
	fmt.Println("shutdown call back 2")
}
@ALTree
Copy link
Member

ALTree commented May 18, 2019 •
edited

Hi @chenjie199234,

this is how Go works. When the main goroutine exists, the program is done. The main goroutine won't wait for other goroutines to end. Once you call return in the main function after <-done triggers, the main function will return without caring if other goroutines still have work to do (in this case, your callbacks).

You'll need some form of synchronization if you want to make main wait for the other goroutines to finish before exiting.

I'm closing this, since this is not a Go bug.

@ALTree ALTree closed this as completed May 18, 2019
@alexedwards
Copy link

@ALTree I've just run into this same issue, and I'm not sure it should have been closed.

The problem is that http.Server.Shutdown() doesn't wait for the functions registered with http.Server.RegisterOnShutdown to complete before returning:

func (srv *Server) Shutdown(ctx context.Context) error {
	atomic.StoreInt32(&srv.inShutdown, 1)

	srv.mu.Lock()
	lnerr := srv.closeListenersLocked()
	srv.closeDoneChanLocked()
	for _, f := range srv.onShutdown {
		go f()
	}
	srv.mu.Unlock()

	ticker := time.NewTicker(shutdownPollInterval)
	defer ticker.Stop()
	for {
		if srv.closeIdleConns() {
			return lnerr
		}
		select {
		case <-ctx.Done():
			return ctx.Err()
		case <-ticker.C:
		}
	}
}

Even if this is intended behaviour of Shutdown() -- and I'm not sure it is -- then it's not intuitive and I think the documentation should be updated to warn that the functions you register with RegisterOnShutdown() may not complete before Shutdown() returns.

@ALTree
Copy link
Member

ALTree commented Oct 15, 2019

Even if this is intended behaviour of Shutdown() -- and I'm not sure it is -- then it's not intuitive and I think the documentation should be updated to warn that the functions you register with RegisterOnShutdown() may not complete before Shutdown() returns.

Fair enough; I'm re-opening this for further investigation (and possibly to be converted to a doc issue).

@ALTree ALTree reopened this Oct 15, 2019
@ALTree ALTree added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Oct 15, 2019
@ALTree ALTree changed the title http server's Shutdown() method is not graceful enough net/http: server's Shutdown() method is not graceful enough Oct 15, 2019
@ALTree
Copy link
Member

ALTree commented Oct 15, 2019

cc @bradfitz @tombergan

@acynothia
Copy link 

for _, f := range srv.onShutdown {
	go f()
}

call onShutdown with goroutine, a WaitGroup maybe work. but it's not intuitive.

@rumsrami
Copy link

any update on this issue?

@rkuska
Copy link
Contributor

rkuska commented Mar 13, 2020 •
edited

Hey! I found this issue when browsing code in net/http. I gave it a try and wrote a quick fix, but it might be too complicated. Is there an interest to have this fixed and is this issue suitable for volunteers (newcomers)?

diff --git a/src/net/http/server.go b/src/net/http/server.go
index 58aff08424..e013e40d2f 100644
--- a/src/net/http/server.go
+++ b/src/net/http/server.go
@@ -2667,19 +2667,41 @@ var shutdownPollInterval = 500 * time.Millisecond
 // future calls to methods such as Serve will return ErrServerClosed.
 func (srv *Server) Shutdown(ctx context.Context) error {
        atomic.StoreInt32(&srv.inShutdown, 1)
+       var wg sync.WaitGroup
+       wgDone := make(chan struct{})

        srv.mu.Lock()
        lnerr := srv.closeListenersLocked()
        srv.closeDoneChanLocked()
+
+       wg.Add(len(srv.onShutdown))
        for _, f := range srv.onShutdown {
-               go f()
+               f := f
+               go func() {
+                       defer wg.Done()
+                       f()
+               }()
        }
        srv.mu.Unlock()

+       go func() {
+               defer close(wgDone)
+               wg.Wait()
+       }()
+
+       isOnShutdownDone := func() bool {
+               select {
+               case <-wgDone:
+                       return true
+               default:
+                       return false
+               }
+       }
+
        ticker := time.NewTicker(shutdownPollInterval)
        defer ticker.Stop()
        for {
-               if srv.closeIdleConns() {
+               if srv.closeIdleConns() && isOnShutdownDone() {
                        return lnerr
                }
                select {

@nhooyr-ts
Copy link

There's another issue here where if you call Shutdown twice, the callbacks will be invoked in a new goroutine twice...

@seankhliao seankhliao added this to the Unplanned milestone Aug 20, 2022
@billopark

This comment was marked as spam.

Sign in to view
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Unplanned
Development

No branches or pull requests
9 participants
@alexedwards @rkuska @ALTree @seankhliao @acynothia @chenjie199234 @billopark @rumsrami @nhooyr-ts