From 79394b340d227182f94bae48cb08a091c78f2ea2 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Wed, 15 Nov 2023 22:02:46 +0800 Subject: [PATCH 1/2] Improve graceful manager code/comment (#28063) The graceful manager has some bugs (#27643, #28062). This is a preparation for further fixes. --- modules/graceful/context.go | 7 ++++ modules/graceful/manager.go | 52 +++++------------------------ modules/graceful/manager_unix.go | 4 ++- modules/graceful/manager_windows.go | 4 ++- modules/graceful/net_unix.go | 8 ++--- modules/graceful/net_windows.go | 4 +-- modules/graceful/server.go | 26 ++------------- modules/graceful/server_http.go | 7 ++-- 8 files changed, 29 insertions(+), 83 deletions(-) diff --git a/modules/graceful/context.go b/modules/graceful/context.go index 4fcbcb04b6a3..c9c4ca4e67ad 100644 --- a/modules/graceful/context.go +++ b/modules/graceful/context.go @@ -7,6 +7,13 @@ import ( "context" ) +// Shutdown procedure: +// * cancel ShutdownContext: the registered context consumers have time to do their cleanup (they could use the hammer context) +// * cancel HammerContext: the all context consumers have limited time to do their cleanup (wait for a few seconds) +// * cancel TerminateContext: the registered context consumers have time to do their cleanup (but they shouldn't use shutdown/hammer context anymore) +// * cancel manager context +// If the shutdown is triggered again during the shutdown procedure, the hammer context will be canceled immediately to force to shut down. + // ShutdownContext returns a context.Context that is Done at shutdown // Callers using this context should ensure that they are registered as a running server // in order that they are waited for. diff --git a/modules/graceful/manager.go b/modules/graceful/manager.go index 068de21076fc..f3f412863ac3 100644 --- a/modules/graceful/manager.go +++ b/modules/graceful/manager.go @@ -39,10 +39,10 @@ type RunCanceler interface { // and add a function to call manager.InformCleanup if it's not going to be used const numberOfServersToCreate = 4 -// Manager represents the graceful server manager interface -var manager *Manager - -var initOnce = sync.Once{} +var ( + manager *Manager + initOnce sync.Once +) // GetManager returns the Manager func GetManager() *Manager { @@ -147,12 +147,12 @@ func (g *Manager) doShutdown() { go g.doHammerTime(setting.GracefulHammerTime) } go func() { - g.WaitForServers() + g.runningServerWaitGroup.Wait() // Mop up any remaining unclosed events. g.doHammerTime(0) <-time.After(1 * time.Second) g.doTerminate() - g.WaitForTerminate() + g.terminateWaitGroup.Wait() g.lock.Lock() g.managerCtxCancel() g.lock.Unlock() @@ -199,26 +199,18 @@ func (g *Manager) IsChild() bool { } // IsShutdown returns a channel which will be closed at shutdown. -// The order of closure is IsShutdown, IsHammer (potentially), IsTerminate +// The order of closure is shutdown, hammer (potentially), terminate func (g *Manager) IsShutdown() <-chan struct{} { return g.shutdownCtx.Done() } -// IsHammer returns a channel which will be closed at hammer -// The order of closure is IsShutdown, IsHammer (potentially), IsTerminate +// IsHammer returns a channel which will be closed at hammer. // Servers running within the running server wait group should respond to IsHammer // if not shutdown already func (g *Manager) IsHammer() <-chan struct{} { return g.hammerCtx.Done() } -// IsTerminate returns a channel which will be closed at terminate -// The order of closure is IsShutdown, IsHammer (potentially), IsTerminate -// IsTerminate will only close once all running servers have stopped -func (g *Manager) IsTerminate() <-chan struct{} { - return g.terminateCtx.Done() -} - // ServerDone declares a running server done and subtracts one from the // running server wait group. Users probably do not want to call this // and should use one of the RunWithShutdown* functions @@ -226,28 +218,7 @@ func (g *Manager) ServerDone() { g.runningServerWaitGroup.Done() } -// WaitForServers waits for all running servers to finish. Users should probably -// instead use AtTerminate or IsTerminate -func (g *Manager) WaitForServers() { - g.runningServerWaitGroup.Wait() -} - -// WaitForTerminate waits for all terminating actions to finish. -// Only the main go-routine should use this -func (g *Manager) WaitForTerminate() { - g.terminateWaitGroup.Wait() -} - -func (g *Manager) getState() state { - g.lock.RLock() - defer g.lock.RUnlock() - return g.state -} - func (g *Manager) setStateTransition(old, new state) bool { - if old != g.getState() { - return false - } g.lock.Lock() if g.state != old { g.lock.Unlock() @@ -258,13 +229,6 @@ func (g *Manager) setStateTransition(old, new state) bool { return true } -func (g *Manager) setState(st state) { - g.lock.Lock() - defer g.lock.Unlock() - - g.state = st -} - // InformCleanup tells the cleanup wait group that we have either taken a listener or will not be taking a listener. // At the moment the total number of servers (numberOfServersToCreate) are pre-defined as a const before global init, // so this function MUST be called if a server is not used. diff --git a/modules/graceful/manager_unix.go b/modules/graceful/manager_unix.go index b1fd6da76dd1..bdf23a4fdefa 100644 --- a/modules/graceful/manager_unix.go +++ b/modules/graceful/manager_unix.go @@ -107,7 +107,9 @@ func (g *Manager) start(ctx context.Context) { defer pprof.SetGoroutineLabels(ctx) // Set the running state & handle signals - g.setState(stateRunning) + if !g.setStateTransition(stateInit, stateRunning) { + panic("invalid graceful manager state: transition from init to running failed") + } g.notify(statusMsg("Starting Gitea")) g.notify(pidMsg()) go g.handleSignals(g.managerCtx) diff --git a/modules/graceful/manager_windows.go b/modules/graceful/manager_windows.go index f676f86d0437..c2ea5383ccc5 100644 --- a/modules/graceful/manager_windows.go +++ b/modules/graceful/manager_windows.go @@ -85,7 +85,9 @@ func (g *Manager) start() { g.shutdownRequested = make(chan struct{}) // Set the running state - g.setState(stateRunning) + if !g.setStateTransition(stateInit, stateRunning) { + panic("invalid graceful manager state: transition from init to running failed") + } if skip, _ := strconv.ParseBool(os.Getenv("SKIP_MINWINSVC")); skip { log.Trace("Skipping SVC check as SKIP_MINWINSVC is set") return diff --git a/modules/graceful/net_unix.go b/modules/graceful/net_unix.go index f5af1e39378a..4f8c036a6961 100644 --- a/modules/graceful/net_unix.go +++ b/modules/graceful/net_unix.go @@ -150,12 +150,8 @@ func CloseProvidedListeners() error { return returnableError } -// DefaultGetListener obtains a listener for the local network address. The network must be -// a stream-oriented network: "tcp", "tcp4", "tcp6", "unix" or "unixpacket". It -// returns an provided net.Listener for the matching network and address, or -// creates a new one using net.Listen. This function can be replaced by changing the -// GetListener variable at the top of this file, for example to listen on an onion service using -// github.com/cretz/bine +// DefaultGetListener obtains a listener for the stream-oriented local network address: +// "tcp", "tcp4", "tcp6", "unix" or "unixpacket". func DefaultGetListener(network, address string) (net.Listener, error) { // Add a deferral to say that we've tried to grab a listener defer GetManager().InformCleanup() diff --git a/modules/graceful/net_windows.go b/modules/graceful/net_windows.go index 15d228d6b654..9667bd4d13e2 100644 --- a/modules/graceful/net_windows.go +++ b/modules/graceful/net_windows.go @@ -10,9 +10,7 @@ package graceful import "net" // DefaultGetListener obtains a listener for the local network address. -// On windows this is basically just a shim around net.Listen. This function -// can be replaced by changing the GetListener variable at the top of this file, -// for example to listen on an onion service using github.com/cretz/bine +// On windows this is basically just a shim around net.Listen. func DefaultGetListener(network, address string) (net.Listener, error) { // Add a deferral to say that we've tried to grab a listener defer GetManager().InformCleanup() diff --git a/modules/graceful/server.go b/modules/graceful/server.go index bd917828bc86..2525a83e7752 100644 --- a/modules/graceful/server.go +++ b/modules/graceful/server.go @@ -20,31 +20,11 @@ import ( "code.gitea.io/gitea/modules/setting" ) -var ( - // DefaultReadTimeOut default read timeout - DefaultReadTimeOut time.Duration - // DefaultWriteTimeOut default write timeout - DefaultWriteTimeOut time.Duration - // DefaultMaxHeaderBytes default max header bytes - DefaultMaxHeaderBytes int - // PerWriteWriteTimeout timeout for writes - PerWriteWriteTimeout = 30 * time.Second - // PerWriteWriteTimeoutKbTime is a timeout taking account of how much there is to be written - PerWriteWriteTimeoutKbTime = 10 * time.Second -) - -// GetListener returns a listener from a GetListener function, which must have the -// signature: `func FunctioName(network, address string) (net.Listener, error)`. -// This determines the implementation of net.Listener which the server will use.` -// It is implemented in this way so that downstreams may specify the type of listener -// they want to provide Gitea on by default, such as with a hidden service or a p2p network -// No need to worry about "breaking" if there would be a refactoring for the Listeners. No compatibility-guarantee for this mechanism +// GetListener returns a net listener +// This determines the implementation of net.Listener which the server will use, +// so that downstreams could provide their own Listener, such as with a hidden service or a p2p network var GetListener = DefaultGetListener -func init() { - DefaultMaxHeaderBytes = 0 // use http.DefaultMaxHeaderBytes - which currently is 1 << 20 (1MB) -} - // ServeFunction represents a listen.Accept loop type ServeFunction = func(net.Listener) error diff --git a/modules/graceful/server_http.go b/modules/graceful/server_http.go index a1f8e0ff522a..7c855ac64ec1 100644 --- a/modules/graceful/server_http.go +++ b/modules/graceful/server_http.go @@ -13,11 +13,8 @@ import ( func newHTTPServer(network, address, name string, handler http.Handler) (*Server, ServeFunction) { server := NewServer(network, address, name) httpServer := http.Server{ - ReadTimeout: DefaultReadTimeOut, - WriteTimeout: DefaultWriteTimeOut, - MaxHeaderBytes: DefaultMaxHeaderBytes, - Handler: handler, - BaseContext: func(net.Listener) context.Context { return GetManager().HammerContext() }, + Handler: handler, + BaseContext: func(net.Listener) context.Context { return GetManager().HammerContext() }, } server.OnShutdown = func() { httpServer.SetKeepAlivesEnabled(false) From 49dddd87b19aebe83e1c54a455e62529a19f61b4 Mon Sep 17 00:00:00 2001 From: sebastian-sauer Date: Thu, 16 Nov 2023 04:58:53 +0100 Subject: [PATCH 2/2] Improve PR diff view on mobile (#27883) 1. Show diff stats only on large screens these are already shown in tabs, so no need for this duplicate information on small screens ![image](https://github.com/go-gitea/gitea/assets/1135157/1287839d-7490-42eb-a17e-d526dc0bfd9e) ![image](https://github.com/go-gitea/gitea/assets/1135157/e9dcd89d-ed4d-4945-a7aa-4e6fc6d9c3a2) 2. Hide viewed files information on small screens Github does the same and this gives us more free space on small screens ![image](https://github.com/go-gitea/gitea/assets/1135157/e90b042f-fffb-4f79-a5ae-cd480c9d8334) ![image](https://github.com/go-gitea/gitea/assets/1135157/d2480ffe-58f2-4694-8ae1-a2ab0aae14d4) 3. Review bar now doesn't wrap so we don't need the 77px even on very small screens (the sticky headers are still working) ![image](https://github.com/go-gitea/gitea/assets/1135157/42b19b2b-73ef-4b88-8680-c555879b363b) --- templates/repo/diff/box.tmpl | 2 +- web_src/css/repo.css | 27 ++------------------------- 2 files changed, 3 insertions(+), 26 deletions(-) diff --git a/templates/repo/diff/box.tmpl b/templates/repo/diff/box.tmpl index 945c521a5773..e72ac1eeaefe 100644 --- a/templates/repo/diff/box.tmpl +++ b/templates/repo/diff/box.tmpl @@ -25,7 +25,7 @@
{{if and .PageIsPullFiles $.SignedUserID (not .IsArchived) (not .DiffNotAvailable)}} -
+
diff --git a/web_src/css/repo.css b/web_src/css/repo.css index d759d10c8a06..6bddbe9ba560 100644 --- a/web_src/css/repo.css +++ b/web_src/css/repo.css @@ -1506,7 +1506,6 @@ @media (max-width: 991.98px) { .repository .diff-detail-box { flex-direction: row; - height: 77px; /* this height should match sticky-2nd-row */ } } @@ -1534,13 +1533,9 @@ color: var(--color-red); } -@media (max-width: 480px) { +@media (max-width: 991.98px) { .repository .diff-detail-box .diff-detail-stats { - font-size: 0; - line-height: 1.6rem; - } - .repository .diff-detail-box .diff-detail-stats strong { - font-size: 1rem; + display: none !important; } } @@ -1735,12 +1730,6 @@ border: none; } -@media (max-width: 991.98px) { - .diff-file-box { - scroll-margin-top: 77px; /* match .repository .diff-detail-box */ - } -} - /* TODO: this can potentially be made "global" by removing the class prefix */ .diff-file-box .ui.attached.header, .diff-file-box .ui.attached.table { @@ -2826,18 +2815,6 @@ tbody.commit-list { z-index: 7; } -@media (max-width: 991.98px) { - .ui.attached.header.diff-file-header.sticky-2nd-row { - top: 77px; /* match .repository .diff-detail-box */ - } -} - -@media (max-width: 480px) { - .ui.attached.header.diff-file-header.sticky-2nd-row { - position: static; - } -} - .diff-file-name { flex: auto; min-width: 100px;