From 803da92ca93c44439eaae8faf0afbbfb4d60928b Mon Sep 17 00:00:00 2001 From: Jakob Borg Date: Wed, 31 May 2017 18:14:04 +0000 Subject: [PATCH] cmd/syncthing: Start CPU usage monitoring not from init (fixes #4183) Starting stuff from init() is an antipattern, and the innerProcess variable isn't 100% reliable. We should sort out the other uses of it as well in due time. Also removing the hack on innerProcess as I happened to see it and the affected versions are now <1% users. GitHub-Pull-Request: https://github.com/syncthing/syncthing/pull/4185 --- cmd/syncthing/cpuusage.go | 59 +++++++++++++++++++++++++++ cmd/syncthing/gui.go | 35 ++++------------ cmd/syncthing/gui_test.go | 7 ++-- cmd/syncthing/main.go | 33 ++------------- cmd/syncthing/mocked_cpuusage_test.go | 13 ++++++ 5 files changed, 88 insertions(+), 59 deletions(-) create mode 100644 cmd/syncthing/cpuusage.go create mode 100644 cmd/syncthing/mocked_cpuusage_test.go diff --git a/cmd/syncthing/cpuusage.go b/cmd/syncthing/cpuusage.go new file mode 100644 index 00000000..4b02c32f --- /dev/null +++ b/cmd/syncthing/cpuusage.go @@ -0,0 +1,59 @@ +// Copyright (C) 2017 The Syncthing Authors. +// +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this file, +// You can obtain one at https://mozilla.org/MPL/2.0/. + +package main + +import ( + "math" + "time" + + metrics "github.com/rcrowley/go-metrics" +) + +const cpuTickRate = 5 * time.Second + +type cpuService struct { + avg metrics.EWMA + stop chan struct{} +} + +func newCPUService() *cpuService { + return &cpuService{ + // 10 second average. Magic alpha value comes from looking at EWMA package + // definitions of EWMA1, EWMA5. The tick rate *must* be five seconds (hard + // coded in the EWMA package). + avg: metrics.NewEWMA(1 - math.Exp(-float64(cpuTickRate)/float64(time.Second)/10.0)), + stop: make(chan struct{}), + } +} + +func (s *cpuService) Serve() { + // Initialize prevUsage to an actual value returned by cpuUsage + // instead of zero, because at least Windows returns a huge negative + // number here that then slowly increments... + prevUsage := cpuUsage() + ticker := time.NewTicker(cpuTickRate) + defer ticker.Stop() + for { + select { + case <-ticker.C: + curUsage := cpuUsage() + s.avg.Update(int64((curUsage - prevUsage) / time.Millisecond)) + prevUsage = curUsage + s.avg.Tick() + case <-s.stop: + return + } + } +} + +func (s *cpuService) Stop() { + close(s.stop) +} + +func (s *cpuService) Rate() float64 { + return s.avg.Rate() +} diff --git a/cmd/syncthing/gui.go b/cmd/syncthing/gui.go index bf2e33fd..c6bd3a96 100644 --- a/cmd/syncthing/gui.go +++ b/cmd/syncthing/gui.go @@ -11,7 +11,6 @@ import ( "encoding/json" "fmt" "io/ioutil" - "math" "net" "net/http" "os" @@ -69,6 +68,7 @@ type apiService struct { configChanged chan struct{} // signals intentional listener close due to config change started chan string // signals startup complete by sending the listener address, for testing only startedOnce chan struct{} // the service has started successfully at least once + cpu rater guiErrors logger.Recorder systemLog logger.Recorder @@ -121,7 +121,11 @@ type connectionsIntf interface { Status() map[string]interface{} } -func newAPIService(id protocol.DeviceID, cfg configIntf, httpsCertFile, httpsKeyFile, assetDir string, m modelIntf, defaultSub, diskSub events.BufferedSubscription, discoverer discover.CachingMux, connectionsService connectionsIntf, errors, systemLog logger.Recorder) *apiService { +type rater interface { + Rate() float64 +} + +func newAPIService(id protocol.DeviceID, cfg configIntf, httpsCertFile, httpsKeyFile, assetDir string, m modelIntf, defaultSub, diskSub events.BufferedSubscription, discoverer discover.CachingMux, connectionsService connectionsIntf, errors, systemLog logger.Recorder, cpu rater) *apiService { service := &apiService{ id: id, cfg: cfg, @@ -142,6 +146,7 @@ func newAPIService(id protocol.DeviceID, cfg configIntf, httpsCertFile, httpsKey startedOnce: make(chan struct{}), guiErrors: errors, systemLog: systemLog, + cpu: cpu, } return service @@ -847,30 +852,6 @@ func (s *apiService) flushResponse(resp string, w http.ResponseWriter) { f.Flush() } -// 10 second average. Magic alpha value comes from looking at EWMA package -// definitions of EWMA1, EWMA5. The tick rate *must* be five seconds (hard -// coded in the EWMA package). -var cpuTickRate = 5 * time.Second -var cpuAverage = metrics.NewEWMA(1 - math.Exp(-float64(cpuTickRate)/float64(time.Second)/10.0)) - -func init() { - if !innerProcess { - return - } - go func() { - // Initialize prevUsage to an actual value returned by cpuUsage - // instead of zero, because at least Windows returns a huge negative - // number here that then slowly increments... - prevUsage := cpuUsage() - for range time.NewTicker(cpuTickRate).C { - curUsage := cpuUsage() - cpuAverage.Update(int64((curUsage - prevUsage) / time.Millisecond)) - prevUsage = curUsage - cpuAverage.Tick() - } - }() -} - func (s *apiService) getSystemStatus(w http.ResponseWriter, r *http.Request) { var m runtime.MemStats runtime.ReadMemStats(&m) @@ -899,7 +880,7 @@ func (s *apiService) getSystemStatus(w http.ResponseWriter, r *http.Request) { res["connectionServiceStatus"] = s.connectionsService.Status() // cpuUsage.Rate() is in milliseconds per second, so dividing by ten // gives us percent - res["cpuPercent"] = cpuAverage.Rate() / 10 / float64(runtime.NumCPU()) + res["cpuPercent"] = s.cpu.Rate() / 10 / float64(runtime.NumCPU()) res["pathSeparator"] = string(filepath.Separator) res["uptime"] = int(time.Since(startTime).Seconds()) res["startTime"] = startTime diff --git a/cmd/syncthing/gui_test.go b/cmd/syncthing/gui_test.go index 92e2df7f..f89a22dc 100644 --- a/cmd/syncthing/gui_test.go +++ b/cmd/syncthing/gui_test.go @@ -71,7 +71,7 @@ func TestStopAfterBrokenConfig(t *testing.T) { } w := config.Wrap("/dev/null", cfg) - srv := newAPIService(protocol.LocalDeviceID, w, "../../test/h1/https-cert.pem", "../../test/h1/https-key.pem", "", nil, nil, nil, nil, nil, nil, nil) + srv := newAPIService(protocol.LocalDeviceID, w, "../../test/h1/https-cert.pem", "../../test/h1/https-key.pem", "", nil, nil, nil, nil, nil, nil, nil, nil) srv.started = make(chan string) sup := suture.NewSimple("test") @@ -475,11 +475,12 @@ func startHTTP(cfg *mockedConfig) (string, error) { connections := new(mockedConnections) errorLog := new(mockedLoggerRecorder) systemLog := new(mockedLoggerRecorder) + cpu := new(mockedCPUService) addrChan := make(chan string) // Instantiate the API service svc := newAPIService(protocol.LocalDeviceID, cfg, httpsCertFile, httpsKeyFile, assetDir, model, - eventSub, diskEventSub, discoverer, connections, errorLog, systemLog) + eventSub, diskEventSub, discoverer, connections, errorLog, systemLog, cpu) svc.started = addrChan // Actually start the API service @@ -930,7 +931,7 @@ func TestEventMasks(t *testing.T) { cfg := new(mockedConfig) defSub := new(mockedEventSub) diskSub := new(mockedEventSub) - svc := newAPIService(protocol.LocalDeviceID, cfg, "", "", "", nil, defSub, diskSub, nil, nil, nil, nil) + svc := newAPIService(protocol.LocalDeviceID, cfg, "", "", "", nil, defSub, diskSub, nil, nil, nil, nil, nil) if mask := svc.getEventMask(""); mask != defaultEventMask { t.Errorf("incorrect default mask %x != %x", int64(mask), int64(defaultEventMask)) diff --git a/cmd/syncthing/main.go b/cmd/syncthing/main.go index 39d8fa3f..058df843 100644 --- a/cmd/syncthing/main.go +++ b/cmd/syncthing/main.go @@ -427,34 +427,6 @@ func main() { return } - // ---BEGIN TEMPORARY HACK--- - // - // Remove once v0.14.21-v0.14.22 are rare enough. Those versions, - // essentially: - // - // 1. os.Setenv("STMONITORED", "yes") - // 2. os.Setenv("STNORESTART", "") - // - // where the intention was for 2 to cancel out 1 instead of setting - // STNORESTART to the empty value. We check for exactly this combination - // and pretend that neither was set. Looking through os.Environ lets us - // distinguish. Luckily, we weren't smart enough to use os.Unsetenv. - - matches := 0 - for _, str := range os.Environ() { - if str == "STNORESTART=" { - matches++ - } - if str == "STMONITORED=yes" { - matches++ - } - } - if matches == 2 { - innerProcess = false - } - - // ---END TEMPORARY HACK--- - if innerProcess || options.noRestart { syncthingMain(options) } else { @@ -1093,7 +1065,10 @@ func setupGUI(mainService *suture.Supervisor, cfg *config.Wrapper, m *model.Mode l.Warnln("Insecure admin access is enabled.") } - api := newAPIService(myID, cfg, locations[locHTTPSCertFile], locations[locHTTPSKeyFile], runtimeOptions.assetDir, m, defaultSub, diskSub, discoverer, connectionsService, errors, systemLog) + cpu := newCPUService() + mainService.Add(cpu) + + api := newAPIService(myID, cfg, locations[locHTTPSCertFile], locations[locHTTPSKeyFile], runtimeOptions.assetDir, m, defaultSub, diskSub, discoverer, connectionsService, errors, systemLog, cpu) cfg.Subscribe(api) mainService.Add(api) diff --git a/cmd/syncthing/mocked_cpuusage_test.go b/cmd/syncthing/mocked_cpuusage_test.go new file mode 100644 index 00000000..b45a44a7 --- /dev/null +++ b/cmd/syncthing/mocked_cpuusage_test.go @@ -0,0 +1,13 @@ +// Copyright (C) 2017 The Syncthing Authors. +// +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this file, +// You can obtain one at https://mozilla.org/MPL/2.0/. + +package main + +type mockedCPUService struct{} + +func (*mockedCPUService) Rate() float64 { + return 42 +}