From b80fe58e4d259a4b2bd72a04173bfb492b19e6f6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E4=B8=8D=E6=8F=92=E7=94=B5?= Date: Tue, 26 Nov 2024 01:13:57 +0800 Subject: [PATCH 1/3] fix: ShutdownWithContext and ctx.Done() exist race. --- server.go | 17 +++++++---------- server_race_test.go | 46 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 53 insertions(+), 10 deletions(-) create mode 100644 server_race_test.go diff --git a/server.go b/server.go index bcf30cb9e3..377fa14dc0 100644 --- a/server.go +++ b/server.go @@ -1887,6 +1887,8 @@ func (s *Server) Shutdown() error { // // ShutdownWithContext does not close keepalive connections so it's recommended to set ReadTimeout and IdleTimeout // to something else than 0. +// +// When ShutdownWithContext returns errors, any operation to the Server is unavailable. func (s *Server) ShutdownWithContext(ctx context.Context) (err error) { s.mu.Lock() defer s.mu.Unlock() @@ -1932,7 +1934,10 @@ END: } } - s.done = nil + // There may be a surviving request to call ctx.Done(). + if err == nil { + s.done = nil + } s.ln = nil return err } @@ -2749,15 +2754,7 @@ func (ctx *RequestCtx) Deadline() (deadline time.Time, ok bool) { // Note: Because creating a new channel for every request is just too expensive, so // RequestCtx.s.done is only closed when the server is shutting down. func (ctx *RequestCtx) Done() <-chan struct{} { - // fix use new variables to prevent panic caused by modifying the original done chan to nil. - done := ctx.s.done - - if done == nil { - done = make(chan struct{}, 1) - done <- struct{}{} - return done - } - return done + return ctx.s.done } // Err returns a non-nil error value after Done is closed, diff --git a/server_race_test.go b/server_race_test.go new file mode 100644 index 0000000000..a409cd804e --- /dev/null +++ b/server_race_test.go @@ -0,0 +1,46 @@ +//go:build race + +package fasthttp + +import ( + "context" + "github.com/valyala/fasthttp/fasthttputil" + "math" + "testing" +) + +func TestServerDoneRace(t *testing.T) { + t.Parallel() + + s := &Server{ + Handler: func(ctx *RequestCtx) { + for i := 0; i < math.MaxInt; i++ { + ctx.Done() + } + }, + } + + ln := fasthttputil.NewInmemoryListener() + defer ln.Close() + + go func() { + if err := s.Serve(ln); err != nil { + t.Errorf("unexpected error: %v", err) + } + }() + + c, err := ln.Dial() + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + defer c.Close() + if _, err = c.Write([]byte("POST / HTTP/1.1\r\nHost: go.dev\r\nContent-Length: 3\r\n\r\nABC" + + "\r\n\r\n" + // <-- this stuff is bogus, but we'll ignore it + "GET / HTTP/1.1\r\nHost: go.dev\r\n\r\n")); err != nil { + t.Fatal(err) + } + ctx, cancelFunc := context.WithCancel(context.Background()) + cancelFunc() + + s.ShutdownWithContext(ctx) +} From 7b151deaa599ec02bf1a775095fac21709e64183 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E4=B8=8D=E6=8F=92=E7=94=B5?= Date: Tue, 26 Nov 2024 01:25:44 +0800 Subject: [PATCH 2/3] fix: Even if ln.Close() err, the Shutdown process should still proceed. --- server.go | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/server.go b/server.go index 377fa14dc0..907bf9ec95 100644 --- a/server.go +++ b/server.go @@ -1900,11 +1900,7 @@ func (s *Server) ShutdownWithContext(ctx context.Context) (err error) { return nil } - for _, ln := range s.ln { - if err = ln.Close(); err != nil { - return err - } - } + lnerr := s.closeListenersLocked() if s.done != nil { close(s.done) @@ -1920,6 +1916,9 @@ END: s.closeIdleConns() if open := atomic.LoadInt32(&s.open); open == 0 { + err = lnerr + // There may be a pending request to call ctx.Done(). Therefore, we only set it to nil when open == 0. + s.done = nil break } // This is not an optimal solution but using a sync.WaitGroup @@ -1934,10 +1933,6 @@ END: } } - // There may be a surviving request to call ctx.Done(). - if err == nil { - s.done = nil - } s.ln = nil return err } @@ -2931,6 +2926,16 @@ func (s *Server) closeIdleConns() { s.idleConnsMu.Unlock() } +func (s *Server) closeListenersLocked() error { + var err error + for _, ln := range s.ln { + if cerr := ln.Close(); cerr != nil && err == nil { + err = cerr + } + } + return err +} + // A ConnState represents the state of a client connection to a server. // It's used by the optional Server.ConnState hook. type ConnState int From aa1ff536cf2fefcbf1c3e1f082cf96811a6ee934 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E4=B8=8D=E6=8F=92=E7=94=B5?= Date: Tue, 26 Nov 2024 01:42:15 +0800 Subject: [PATCH 3/3] refactor: remove END label. --- server.go | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/server.go b/server.go index 907bf9ec95..4e44122454 100644 --- a/server.go +++ b/server.go @@ -1911,30 +1911,25 @@ func (s *Server) ShutdownWithContext(ctx context.Context) (err error) { // Now we just have to wait until all workers are done or timeout. ticker := time.NewTicker(time.Millisecond * 100) defer ticker.Stop() -END: + for { s.closeIdleConns() if open := atomic.LoadInt32(&s.open); open == 0 { - err = lnerr // There may be a pending request to call ctx.Done(). Therefore, we only set it to nil when open == 0. s.done = nil - break + return lnerr } // This is not an optimal solution but using a sync.WaitGroup // here causes data races as it's hard to prevent Add() to be called // while Wait() is waiting. select { case <-ctx.Done(): - err = ctx.Err() - break END + return ctx.Err() case <-ticker.C: continue } } - - s.ln = nil - return err } func acceptConn(s *Server, ln net.Listener, lastPerIPErrorTime *time.Time) (net.Conn, error) { @@ -2933,6 +2928,7 @@ func (s *Server) closeListenersLocked() error { err = cerr } } + s.ln = nil return err }