Repository: valyala/fasthttp
Pull request #755 (2020-03-03 08:21:31)

Make the prefork mode more robust
The main process will exit if one of the prefork child processes doesn't complete successfully under the current prefork mode, so it ought to make sure that all child processes run independently and the main process will only exit after all child processes are finished.

panjf2000: /cc @erikdubbelboer 
savsgio: HI @panjf2000,

It's not necessary to wait to the all child procs will finish, because if the master process dies, the children should die too.
Also, you are collected all error and just return the first one.

```go
return p.prefork(addr)[0]
```

Think that, if you wait to all child procs dies/end, you will lose performance because maybe you couldn't know it, but if master dies when any of child procs dies, you could know quickly that the server is dead and not lose performance due to the some cores doesn't work.

But we wait for @erikdubbelboer, to know their opinion.
panjf2000: > It's not necessary to wait to the all child procs will finish, because if the master process dies, the children should die too.

I know that, but this PR doesn't intend to make sure all child processes to exit after the main process exit, it try to avoid that any child process to break the main process.

> Also, you are collected all error and just return the first one.

Yes, you are right but the root cause of it is that the definition of `ListenAndServe` only return one single `error` so I have to pick only one error to return:
https://github.com/valyala/fasthttp/blob/695f713fcf594b33fd8c126cfa8dc9f119d4391b/server.go#L88
also, it printed those else errors to stdout, unfortunately it is only way for now.

> Think that, if you wait to all child procs dies/end, you will lose performance because maybe you couldn't know it, but if master dies when any of child procs dies, you could know quickly that the server is dead and not lose performance due to the some cores doesn't work.

You got a point but in my opinion, it makes more sense to choose robustness over performance in this case because fasthttp is fast enough, it can still provide high-performance services even though it does lose a little bit of performance because of some child prefork processes breaking down, so it shouldn't shut the entire server down just because one or two prefork child processes break down.

@savsgio 
savsgio: What do you think @erikdubbelboer ?
erikdubbelboer: I think you're both wrong 😄 . When a parent process dies doesn't mean the children also automatically dies: https://unix.stackexchange.com/questions/158727/is-there-any-unix-variant-on-which-a-child-process-dies-with-its-parent

I think if one child dies it should be restarted. Up to a certain (configuratble) limit at which point `ListenAndServe` should return with an error. But before that, it should clean up/kill all the children. That way developers using this can handle failure in the way that they would like.
panjf2000: > When a parent process dies doesn't mean the children also automatically dies: https://unix.stackexchange.com/questions/158727/is-there-any-unix-variant-on-which-a-child-process-dies-with-its-parent

It is right in general, but we were talking about goroutines, you see, in the previous code the `cmd.Run()` was wrapped into a goroutine, so when the goroutine that starts other goroutines dies, other goroutines die, then the child processes started in goroutines become orphan processes and adopted by init process, which I think it is also a bad thing.

> I think if one child dies it should be restarted. Up to a certain (configuratble) limit at which point ListenAndServe should return with an error. But before that, it should clean up/kill all the children. That way developers using this can handle failure in the way that they would like.

So you are suggesting that I should add code that restarts child processes automatically when it finds any child process dies?
erikdubbelboer: Isn't it better if RecoverThreshold has been reached for the master process to kill all the remaining children before returning?
panjf2000: > Isn't it better if RecoverThreshold has been reached for the master process to kill all the remaining children before returning?

What's the problem of killing them in `defer`?
panjf2000: Any progress？
@erikdubbelboer 

Patch:
diff --git a/header.go b/header.go
index b7a12ee..81d2eba 100644
--- a/header.go
+++ b/header.go
@@ -25,6 +25,7 @@ type ResponseHeader struct {
 	noHTTP11             bool
 	connectionClose      bool
 	noDefaultContentType bool
+	noDefaultDate        bool
 
 	statusCode         int
 	contentLength      int
@@ -641,6 +642,7 @@ func (h *ResponseHeader) DisableNormalizing() {
 func (h *ResponseHeader) Reset() {
 	h.disableNormalizing = false
 	h.noDefaultContentType = false
+	h.noDefaultDate = false
 	h.resetSkipNormalize()
 }
 
@@ -694,6 +696,7 @@ func (h *ResponseHeader) CopyTo(dst *ResponseHeader) {
 	dst.noHTTP11 = h.noHTTP11
 	dst.connectionClose = h.connectionClose
 	dst.noDefaultContentType = h.noDefaultContentType
+	dst.noDefaultDate = h.noDefaultDate
 
 	dst.statusCode = h.statusCode
 	dst.contentLength = h.contentLength
@@ -1499,8 +1502,10 @@ func (h *ResponseHeader) AppendBytes(dst []byte) []byte {
 		dst = appendHeaderLine(dst, strServer, server)
 	}
 
-	serverDateOnce.Do(updateServerDate)
-	dst = appendHeaderLine(dst, strDate, serverDate.Load().([]byte))
+	if !h.noDefaultDate {
+		serverDateOnce.Do(updateServerDate)
+		dst = appendHeaderLine(dst, strDate, serverDate.Load().([]byte))
+	}
 
 	// Append Content-Type only for non-zero responses
 	// or if it is explicitly set.
@@ -1518,7 +1523,7 @@ func (h *ResponseHeader) AppendBytes(dst []byte) []byte {
 
 	for i, n := 0, len(h.h); i < n; i++ {
 		kv := &h.h[i]
-		if !bytes.Equal(kv.key, strDate) {
+		if h.noDefaultDate || !bytes.Equal(kv.key, strDate) {
 			dst = appendHeaderLine(dst, kv.key, kv.value)
 		}
 	}
diff --git a/prefork/prefork.go b/prefork/prefork.go
index b7a1470..0b31ef5 100644
--- a/prefork/prefork.go
+++ b/prefork/prefork.go
@@ -1,7 +1,9 @@
 package prefork
 
 import (
+	"errors"
 	"flag"
+	"log"
 	"net"
 	"os"
 	"os/exec"
@@ -11,8 +13,23 @@ import (
 	"github.com/valyala/fasthttp/reuseport"
 )
 
-const preforkChildFlag = "-prefork-child"
-const defaultNetwork = "tcp4"
+const (
+	preforkChildFlag = "-prefork-child"
+	defaultNetwork   = "tcp4"
+)
+
+var (
+	defaultLogger = Logger(log.New(os.Stderr, "", log.LstdFlags))
+	// ErrOverRecovery is returned when the times of starting over child prefork processes exceed
+	// the threshold.
+	ErrOverRecovery = errors.New("exceeding the value of RecoverThreshold")
+)
+
+// Logger is used for logging formatted messages.
+type Logger interface {
+	// Printf must have the same semantics as log.Printf.
+	Printf(format string, args ...interface{})
+}
 
 // Prefork implements fasthttp server prefork
 //
@@ -34,6 +51,13 @@ type Prefork struct {
 	// It's disabled by default
 	Reuseport bool
 
+	// Child prefork processes may exit with failure and will be started over until the times reach
+	// the value of RecoverThreshold, then it will return and terminate the server.
+	RecoverThreshold int
+
+	// By default standard logger from log package is used.
+	Logger Logger
+
 	ServeFunc         func(ln net.Listener) error
 	ServeTLSFunc      func(ln net.Listener, certFile, keyFile string) error
 	ServeTLSEmbedFunc func(ln net.Listener, certData, keyData []byte) error
@@ -63,12 +87,21 @@ func IsChild() bool {
 func New(s *fasthttp.Server) *Prefork {
 	return &Prefork{
 		Network:           defaultNetwork,
+		RecoverThreshold:  runtime.GOMAXPROCS(0) / 2,
+		Logger:            s.Logger,
 		ServeFunc:         s.Serve,
 		ServeTLSFunc:      s.ServeTLS,
 		ServeTLSEmbedFunc: s.ServeTLSEmbed,
 	}
 }
 
+func (p *Prefork) logger() Logger {
+	if p.Logger != nil {
+		return p.Logger
+	}
+	return defaultLogger
+}
+
 func (p *Prefork) listen(addr string) (net.Listener, error) {
 	runtime.GOMAXPROCS(1)
 
@@ -110,30 +143,84 @@ func (p *Prefork) setTCPListenerFiles(addr string) error {
 	return nil
 }
 
-func (p *Prefork) prefork(addr string) error {
-	chErr := make(chan error, 1)
+func (p *Prefork) doCommand() (*exec.Cmd, error) {
+	/* #nosec G204 */
+	cmd := exec.Command(os.Args[0], append(os.Args[1:], preforkChildFlag)...)
+	cmd.Stdout = os.Stdout
+	cmd.Stderr = os.Stderr
+	cmd.ExtraFiles = p.files
+	return cmd, cmd.Start()
+}
 
+func (p *Prefork) prefork(addr string) (err error) {
 	if !p.Reuseport {
-		if err := p.setTCPListenerFiles(addr); err != nil {
-			return err
+		if err = p.setTCPListenerFiles(addr); err != nil {
+			return
+		}
+
+		// defer for closing the net.Listener opened by setTCPListenerFiles.
+		defer func() {
+			e := p.ln.Close()
+			if err == nil {
+				err = e
+			}
+		}()
+	}
+
+	type procSig struct {
+		pid int
+		err error
+	}
+
+	goMaxProcs := runtime.GOMAXPROCS(0)
+	sigCh := make(chan procSig, goMaxProcs)
+	childProcs := make(map[int]*exec.Cmd)
+
+	defer func() {
+		for _, proc := range childProcs {
+			_ = proc.Process.Kill()
+		}
+	}()
+
+	for i := 0; i < goMaxProcs; i++ {
+		var cmd *exec.Cmd
+		if cmd, err = p.doCommand(); err != nil {
+			p.logger().Printf("failed to start a child prefork process, error: %v\n", err)
+			return
 		}
 
-		defer p.ln.Close()
+		childProcs[cmd.Process.Pid] = cmd
+		go func() {
+			sigCh <- procSig{cmd.Process.Pid, cmd.Wait()}
+		}()
 	}
 
-	for i := 0; i < runtime.GOMAXPROCS(0); i++ {
-		/* #nosec G204 */
-		cmd := exec.Command(os.Args[0], append(os.Args[1:], preforkChildFlag)...)
-		cmd.Stdout = os.Stdout
-		cmd.Stderr = os.Stderr
-		cmd.ExtraFiles = p.files
+	var exitedProcs int
+	for sig := range sigCh {
+		delete(childProcs, sig.pid)
+
+		p.logger().Printf("one of the child prefork processes exited with "+
+			"error: %v", sig.err)
 
+		if exitedProcs++; exitedProcs > p.RecoverThreshold {
+			p.logger().Printf("child prefork processes exit too many times, "+
+				"which exceeds the value of RecoverThreshold(%d), "+
+				"exiting the master process.\n", exitedProcs)
+			err = ErrOverRecovery
+			break
+		}
+
+		var cmd *exec.Cmd
+		if cmd, err = p.doCommand(); err != nil {
+			break
+		}
+		childProcs[cmd.Process.Pid] = cmd
 		go func() {
-			chErr <- cmd.Run()
+			sigCh <- procSig{cmd.Process.Pid, cmd.Wait()}
 		}()
 	}
 
-	return <-chErr
+	return
 }
 
 // ListenAndServe serves HTTP requests from the given TCP addr
diff --git a/server.go b/server.go
index 5e2c75f..5c6c96c 100644
--- a/server.go
+++ b/server.go
@@ -322,6 +322,13 @@ type Server struct {
 	// value is explicitly provided during a request.
 	NoDefaultServerHeader bool
 
+	// NoDefaultDate, when set to true, causes the default Date
+	// header to be excluded from the Response.
+	//
+	// The default Date header value is the current date value. When
+	// set to true, the Date will not be present.
+	NoDefaultDate bool
+
 	// NoDefaultContentType, when set to true, causes the default Content-Type
 	// header to be excluded from the Response.
 	//
@@ -876,16 +883,21 @@ func (ctx *RequestCtx) FormFile(key string) (*multipart.FileHeader, error) {
 var ErrMissingFile = errors.New("there is no uploaded file associated with the given key")
 
 // SaveMultipartFile saves multipart file fh under the given filename path.
-func SaveMultipartFile(fh *multipart.FileHeader, path string) error {
-	f, err := fh.Open()
+func SaveMultipartFile(fh *multipart.FileHeader, path string) (err error) {
+	var (
+		f  multipart.File
+		ff *os.File
+	)
+	f, err = fh.Open()
 	if err != nil {
-		return err
+		return
 	}
 
-	if ff, ok := f.(*os.File); ok {
+	var ok bool
+	if ff, ok = f.(*os.File); ok {
 		// Windows can't rename files that are opened.
-		if err := f.Close(); err != nil {
-			return err
+		if err = f.Close(); err != nil {
+			return
 		}
 
 		// If renaming fails we try the normal copying method.
@@ -895,21 +907,29 @@ func SaveMultipartFile(fh *multipart.FileHeader, path string) error {
 		}
 
 		// Reopen f for the code below.
-		f, err = fh.Open()
-		if err != nil {
-			return err
+		if f, err = fh.Open(); err != nil {
+			return
 		}
 	}
 
-	defer f.Close()
+	defer func() {
+		e := f.Close()
+		if err == nil {
+			err = e
+		}
+	}()
 
-	ff, err := os.Create(path)
-	if err != nil {
-		return err
+	if ff, err = os.Create(path); err != nil {
+		return
 	}
-	defer ff.Close()
+	defer func() {
+		e := ff.Close()
+		if err == nil {
+			err = e
+		}
+	}()
 	_, err = copyZeroAlloc(ff, f)
-	return err
+	return
 }
 
 // FormValue returns form value associated with the given key.
@@ -1938,6 +1958,7 @@ func (s *Server) serveConn(c net.Conn) (err error) {
 
 		ctx.Request.isTLS = isTLS
 		ctx.Response.Header.noDefaultContentType = s.NoDefaultContentType
+		ctx.Response.Header.noDefaultDate = s.NoDefaultDate
 
 		if err == nil {
 			if s.ReadTimeout > 0 {
@@ -2500,16 +2521,20 @@ func (s *Server) writeFastError(w io.Writer, statusCode int, msg string) {
 		server = fmt.Sprintf("Server: %s\r\n", s.getServerName())
 	}
 
-	serverDateOnce.Do(updateServerDate)
+	date := ""
+	if !s.NoDefaultDate {
+		serverDateOnce.Do(updateServerDate)
+		date = fmt.Sprintf("Date: %s\r\n", serverDate.Load())
+	}
 
 	fmt.Fprintf(w, "Connection: close\r\n"+
 		server+
-		"Date: %s\r\n"+
+		date+
 		"Content-Type: text/plain\r\n"+
 		"Content-Length: %d\r\n"+
 		"\r\n"+
 		"%s",
-		serverDate.Load(), len(msg), msg)
+		len(msg), msg)
 }
 
 func defaultErrorHandler(ctx *RequestCtx, err error) {


Test patch:
diff --git a/header_test.go b/header_test.go
index 7f84a63..b9b6f46 100644
--- a/header_test.go
+++ b/header_test.go
@@ -1250,6 +1250,20 @@ func TestResponseContentTypeNoDefaultNotEmpty(t *testing.T) {
 	}
 }
 
+func TestResponseDateNoDefaultNotEmpty(t *testing.T) {
+	t.Parallel()
+
+	var h ResponseHeader
+
+	h.noDefaultDate = true
+
+	headers := h.String()
+
+	if strings.Contains(headers, "\r\nDate: ") {
+		t.Fatalf("ResponseDateNoDefaultNotEmpty fail, response: \n%+v\noutcome: \n%q\n", h, headers) //nolint:govet
+	}
+}
+
 func TestRequestHeaderConnectionClose(t *testing.T) {
 	t.Parallel()
 
diff --git a/server_test.go b/server_test.go
index d8b265b..0386f53 100644
--- a/server_test.go
+++ b/server_test.go
@@ -271,6 +271,7 @@ func TestServerName(t *testing.T) {
 		},
 		NoDefaultServerHeader: true,
 		NoDefaultContentType:  true,
+		NoDefaultDate:         true,
 	}
 
 	resp = getReponse()
@@ -281,6 +282,10 @@ func TestServerName(t *testing.T) {
 	if bytes.Contains(resp, []byte("\r\nContent-Type: ")) {
 		t.Fatalf("Unexpected response %q expected no Content-Type header", resp)
 	}
+
+	if bytes.Contains(resp, []byte("\r\nDate: ")) {
+		t.Fatalf("Unexpected response %q expected no Date header", resp)
+	}
 }
 
 func TestRequestCtxString(t *testing.T) {