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

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

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

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

chenjie199234 opened this issue May 17, 2019 · 7 comments

Comments

@chenjie199234
Copy link

@chenjie199234 chenjie199234 commented May 17, 2019

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 ALTree commented May 18, 2019

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 May 18, 2019
@alexedwards
Copy link

@alexedwards alexedwards commented Oct 15, 2019

@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 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 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 ALTree commented Oct 15, 2019

@Astone-Chou
Copy link

@Astone-Chou Astone-Chou commented Nov 28, 2019

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

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

@rumsrami
Copy link

@rumsrami rumsrami commented Nov 28, 2019

any update on this issue?

@rkuska
Copy link
Contributor

@rkuska rkuska commented Mar 13, 2020

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 {
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants
You can’t perform that action at this time.