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

@@ -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()
+}

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

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))

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)
 

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
+}