Skip to content

Commit 79394b3

Browse files
authored
Improve graceful manager code/comment (#28063)
The graceful manager has some bugs (#27643, #28062). This is a preparation for further fixes.
1 parent f65977d commit 79394b3

File tree

8 files changed

+29
-83
lines changed

8 files changed

+29
-83
lines changed

modules/graceful/context.go

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,13 @@ import (
77
"context"
88
)
99

10+
// Shutdown procedure:
11+
// * cancel ShutdownContext: the registered context consumers have time to do their cleanup (they could use the hammer context)
12+
// * cancel HammerContext: the all context consumers have limited time to do their cleanup (wait for a few seconds)
13+
// * cancel TerminateContext: the registered context consumers have time to do their cleanup (but they shouldn't use shutdown/hammer context anymore)
14+
// * cancel manager context
15+
// If the shutdown is triggered again during the shutdown procedure, the hammer context will be canceled immediately to force to shut down.
16+
1017
// ShutdownContext returns a context.Context that is Done at shutdown
1118
// Callers using this context should ensure that they are registered as a running server
1219
// in order that they are waited for.

modules/graceful/manager.go

Lines changed: 8 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -39,10 +39,10 @@ type RunCanceler interface {
3939
// and add a function to call manager.InformCleanup if it's not going to be used
4040
const numberOfServersToCreate = 4
4141

42-
// Manager represents the graceful server manager interface
43-
var manager *Manager
44-
45-
var initOnce = sync.Once{}
42+
var (
43+
manager *Manager
44+
initOnce sync.Once
45+
)
4646

4747
// GetManager returns the Manager
4848
func GetManager() *Manager {
@@ -147,12 +147,12 @@ func (g *Manager) doShutdown() {
147147
go g.doHammerTime(setting.GracefulHammerTime)
148148
}
149149
go func() {
150-
g.WaitForServers()
150+
g.runningServerWaitGroup.Wait()
151151
// Mop up any remaining unclosed events.
152152
g.doHammerTime(0)
153153
<-time.After(1 * time.Second)
154154
g.doTerminate()
155-
g.WaitForTerminate()
155+
g.terminateWaitGroup.Wait()
156156
g.lock.Lock()
157157
g.managerCtxCancel()
158158
g.lock.Unlock()
@@ -199,55 +199,26 @@ func (g *Manager) IsChild() bool {
199199
}
200200

201201
// IsShutdown returns a channel which will be closed at shutdown.
202-
// The order of closure is IsShutdown, IsHammer (potentially), IsTerminate
202+
// The order of closure is shutdown, hammer (potentially), terminate
203203
func (g *Manager) IsShutdown() <-chan struct{} {
204204
return g.shutdownCtx.Done()
205205
}
206206

207-
// IsHammer returns a channel which will be closed at hammer
208-
// The order of closure is IsShutdown, IsHammer (potentially), IsTerminate
207+
// IsHammer returns a channel which will be closed at hammer.
209208
// Servers running within the running server wait group should respond to IsHammer
210209
// if not shutdown already
211210
func (g *Manager) IsHammer() <-chan struct{} {
212211
return g.hammerCtx.Done()
213212
}
214213

215-
// IsTerminate returns a channel which will be closed at terminate
216-
// The order of closure is IsShutdown, IsHammer (potentially), IsTerminate
217-
// IsTerminate will only close once all running servers have stopped
218-
func (g *Manager) IsTerminate() <-chan struct{} {
219-
return g.terminateCtx.Done()
220-
}
221-
222214
// ServerDone declares a running server done and subtracts one from the
223215
// running server wait group. Users probably do not want to call this
224216
// and should use one of the RunWithShutdown* functions
225217
func (g *Manager) ServerDone() {
226218
g.runningServerWaitGroup.Done()
227219
}
228220

229-
// WaitForServers waits for all running servers to finish. Users should probably
230-
// instead use AtTerminate or IsTerminate
231-
func (g *Manager) WaitForServers() {
232-
g.runningServerWaitGroup.Wait()
233-
}
234-
235-
// WaitForTerminate waits for all terminating actions to finish.
236-
// Only the main go-routine should use this
237-
func (g *Manager) WaitForTerminate() {
238-
g.terminateWaitGroup.Wait()
239-
}
240-
241-
func (g *Manager) getState() state {
242-
g.lock.RLock()
243-
defer g.lock.RUnlock()
244-
return g.state
245-
}
246-
247221
func (g *Manager) setStateTransition(old, new state) bool {
248-
if old != g.getState() {
249-
return false
250-
}
251222
g.lock.Lock()
252223
if g.state != old {
253224
g.lock.Unlock()
@@ -258,13 +229,6 @@ func (g *Manager) setStateTransition(old, new state) bool {
258229
return true
259230
}
260231

261-
func (g *Manager) setState(st state) {
262-
g.lock.Lock()
263-
defer g.lock.Unlock()
264-
265-
g.state = st
266-
}
267-
268232
// InformCleanup tells the cleanup wait group that we have either taken a listener or will not be taking a listener.
269233
// At the moment the total number of servers (numberOfServersToCreate) are pre-defined as a const before global init,
270234
// so this function MUST be called if a server is not used.

modules/graceful/manager_unix.go

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -107,7 +107,9 @@ func (g *Manager) start(ctx context.Context) {
107107
defer pprof.SetGoroutineLabels(ctx)
108108

109109
// Set the running state & handle signals
110-
g.setState(stateRunning)
110+
if !g.setStateTransition(stateInit, stateRunning) {
111+
panic("invalid graceful manager state: transition from init to running failed")
112+
}
111113
g.notify(statusMsg("Starting Gitea"))
112114
g.notify(pidMsg())
113115
go g.handleSignals(g.managerCtx)

modules/graceful/manager_windows.go

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -85,7 +85,9 @@ func (g *Manager) start() {
8585
g.shutdownRequested = make(chan struct{})
8686

8787
// Set the running state
88-
g.setState(stateRunning)
88+
if !g.setStateTransition(stateInit, stateRunning) {
89+
panic("invalid graceful manager state: transition from init to running failed")
90+
}
8991
if skip, _ := strconv.ParseBool(os.Getenv("SKIP_MINWINSVC")); skip {
9092
log.Trace("Skipping SVC check as SKIP_MINWINSVC is set")
9193
return

modules/graceful/net_unix.go

Lines changed: 2 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -150,12 +150,8 @@ func CloseProvidedListeners() error {
150150
return returnableError
151151
}
152152

153-
// DefaultGetListener obtains a listener for the local network address. The network must be
154-
// a stream-oriented network: "tcp", "tcp4", "tcp6", "unix" or "unixpacket". It
155-
// returns an provided net.Listener for the matching network and address, or
156-
// creates a new one using net.Listen. This function can be replaced by changing the
157-
// GetListener variable at the top of this file, for example to listen on an onion service using
158-
// github.com/cretz/bine
153+
// DefaultGetListener obtains a listener for the stream-oriented local network address:
154+
// "tcp", "tcp4", "tcp6", "unix" or "unixpacket".
159155
func DefaultGetListener(network, address string) (net.Listener, error) {
160156
// Add a deferral to say that we've tried to grab a listener
161157
defer GetManager().InformCleanup()

modules/graceful/net_windows.go

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -10,9 +10,7 @@ package graceful
1010
import "net"
1111

1212
// DefaultGetListener obtains a listener for the local network address.
13-
// On windows this is basically just a shim around net.Listen. This function
14-
// can be replaced by changing the GetListener variable at the top of this file,
15-
// for example to listen on an onion service using github.com/cretz/bine
13+
// On windows this is basically just a shim around net.Listen.
1614
func DefaultGetListener(network, address string) (net.Listener, error) {
1715
// Add a deferral to say that we've tried to grab a listener
1816
defer GetManager().InformCleanup()

modules/graceful/server.go

Lines changed: 3 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -20,31 +20,11 @@ import (
2020
"code.gitea.io/gitea/modules/setting"
2121
)
2222

23-
var (
24-
// DefaultReadTimeOut default read timeout
25-
DefaultReadTimeOut time.Duration
26-
// DefaultWriteTimeOut default write timeout
27-
DefaultWriteTimeOut time.Duration
28-
// DefaultMaxHeaderBytes default max header bytes
29-
DefaultMaxHeaderBytes int
30-
// PerWriteWriteTimeout timeout for writes
31-
PerWriteWriteTimeout = 30 * time.Second
32-
// PerWriteWriteTimeoutKbTime is a timeout taking account of how much there is to be written
33-
PerWriteWriteTimeoutKbTime = 10 * time.Second
34-
)
35-
36-
// GetListener returns a listener from a GetListener function, which must have the
37-
// signature: `func FunctioName(network, address string) (net.Listener, error)`.
38-
// This determines the implementation of net.Listener which the server will use.`
39-
// It is implemented in this way so that downstreams may specify the type of listener
40-
// they want to provide Gitea on by default, such as with a hidden service or a p2p network
41-
// No need to worry about "breaking" if there would be a refactoring for the Listeners. No compatibility-guarantee for this mechanism
23+
// GetListener returns a net listener
24+
// This determines the implementation of net.Listener which the server will use,
25+
// so that downstreams could provide their own Listener, such as with a hidden service or a p2p network
4226
var GetListener = DefaultGetListener
4327

44-
func init() {
45-
DefaultMaxHeaderBytes = 0 // use http.DefaultMaxHeaderBytes - which currently is 1 << 20 (1MB)
46-
}
47-
4828
// ServeFunction represents a listen.Accept loop
4929
type ServeFunction = func(net.Listener) error
5030

modules/graceful/server_http.go

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -13,11 +13,8 @@ import (
1313
func newHTTPServer(network, address, name string, handler http.Handler) (*Server, ServeFunction) {
1414
server := NewServer(network, address, name)
1515
httpServer := http.Server{
16-
ReadTimeout: DefaultReadTimeOut,
17-
WriteTimeout: DefaultWriteTimeOut,
18-
MaxHeaderBytes: DefaultMaxHeaderBytes,
19-
Handler: handler,
20-
BaseContext: func(net.Listener) context.Context { return GetManager().HammerContext() },
16+
Handler: handler,
17+
BaseContext: func(net.Listener) context.Context { return GetManager().HammerContext() },
2118
}
2219
server.OnShutdown = func() {
2320
httpServer.SetKeepAlivesEnabled(false)

0 commit comments

Comments
 (0)