From 1b837116e666f35b7b3099965dbc1eed8acf99de Mon Sep 17 00:00:00 2001 From: Jakob Borg Date: Sat, 11 Jul 2015 10:52:57 +1000 Subject: [PATCH] Let suture logging bubble upwards --- Godeps/Godeps.json | 2 +- .../src/github.com/thejerf/suture/LICENSE | 2 +- .../src/github.com/thejerf/suture/suture.go | 39 ++++++++++++++----- .../github.com/thejerf/suture/suture_test.go | 20 +++++++--- cmd/syncthing/main.go | 4 +- internal/beacon/broadcast.go | 6 --- internal/model/model.go | 8 +--- 7 files changed, 50 insertions(+), 31 deletions(-) diff --git a/Godeps/Godeps.json b/Godeps/Godeps.json index 29d7fe993..08646bf71 100644 --- a/Godeps/Godeps.json +++ b/Godeps/Godeps.json @@ -43,7 +43,7 @@ }, { "ImportPath": "github.com/thejerf/suture", - "Rev": "ff19fb384c3fe30f42717967eaa69da91e5f317c" + "Rev": "fc7aaeabdc43fe41c5328efa1479ffea0b820978" }, { "ImportPath": "github.com/vitrun/qart/coding", diff --git a/Godeps/_workspace/src/github.com/thejerf/suture/LICENSE b/Godeps/_workspace/src/github.com/thejerf/suture/LICENSE index fb1cf4ae3..018bc1c4d 100644 --- a/Godeps/_workspace/src/github.com/thejerf/suture/LICENSE +++ b/Godeps/_workspace/src/github.com/thejerf/suture/LICENSE @@ -1,4 +1,4 @@ -Copyright (c) 2014 Barracuda Networks, Inc. +Copyright (c) 2014-2015 Barracuda Networks, Inc. Permission is hereby granted, free of charge, to any person obtaining a copy of this software and associated documentation files (the "Software"), to deal diff --git a/Godeps/_workspace/src/github.com/thejerf/suture/suture.go b/Godeps/_workspace/src/github.com/thejerf/suture/suture.go index 3d98de13a..8b63d2987 100644 --- a/Godeps/_workspace/src/github.com/thejerf/suture/suture.go +++ b/Godeps/_workspace/src/github.com/thejerf/suture/suture.go @@ -36,6 +36,13 @@ to your Supervisor. Supervisors are also services, so you can create a tree structure here, depending on the exact combination of restarts you want to create. +As a special case, when adding Supervisors to Supervisors, the "sub" +supervisor will have the "super" supervisor's Log function copied. +This allows you to set one log function on the "top" supervisor, and +have it propagate down to all the sub-supervisors. This also allows +libraries or modules to provide Supervisors without having to commit +their users to a particular logging method. + Finally, as what is probably the last line of your main() function, call .Serve() on your top level supervisor. This will start all the services you've defined. @@ -126,8 +133,10 @@ type Supervisor struct { // If you ever come up with some need to get into these, submit a pull // request to make them public and some smidge of justification, and // I'll happily do it. - logBadStop func(Service) - logFailure func(service Service, currentFailures float64, failureThreshold float64, restarting bool, error interface{}, stacktrace []byte) + // But since I've now changed the signature on these once, I'm glad I + // didn't start with them public... :) + logBadStop func(*Supervisor, Service) + logFailure func(supervisor *Supervisor, service Service, currentFailures float64, failureThreshold float64, restarting bool, error interface{}, stacktrace []byte) logBackoff func(*Supervisor, bool) // avoid a dependency on github.com/thejerf/abtime by just implementing @@ -233,10 +242,10 @@ func New(name string, spec Spec) (s *Supervisor) { s.resumeTimer = make(chan time.Time) // set up the default logging handlers - s.logBadStop = func(service Service) { - s.log(fmt.Sprintf("Service %s failed to terminate in a timely manner", serviceName(service))) + s.logBadStop = func(supervisor *Supervisor, service Service) { + s.log(fmt.Sprintf("%s: Service %s failed to terminate in a timely manner", serviceName(supervisor), serviceName(service))) } - s.logFailure = func(service Service, failures float64, threshold float64, restarting bool, err interface{}, st []byte) { + s.logFailure = func(supervisor *Supervisor, service Service, failures float64, threshold float64, restarting bool, err interface{}, st []byte) { var errString string e, canError := err.(error) @@ -246,7 +255,7 @@ func New(name string, spec Spec) (s *Supervisor) { errString = fmt.Sprintf("%#v", err) } - s.log(fmt.Sprintf("Failed service '%s' (%f failures of %f), restarting: %#v, error: %s, stacktrace: %s", serviceName(service), failures, threshold, restarting, errString, string(st))) + s.log(fmt.Sprintf("%s: Failed service '%s' (%f failures of %f), restarting: %#v, error: %s, stacktrace: %s", serviceName(supervisor), serviceName(service), failures, threshold, restarting, errString, string(st))) } s.logBackoff = func(s *Supervisor, entering bool) { if entering { @@ -346,12 +355,24 @@ will be started when the supervisor is. The returned ServiceID may be passed to the Remove method of the Supervisor to terminate the service. + +As a special behavior, if the service added is itself a supervisor, the +supervisor being added will copy the Log function from the Supervisor it +is being added to. This allows factoring out providing a Supervisor +from its logging. + */ func (s *Supervisor) Add(service Service) ServiceToken { if s == nil { panic("can't add service to nil *suture.Supervisor") } + if supervisor, isSupervisor := service.(*Supervisor); isSupervisor { + supervisor.logBadStop = s.logBadStop + supervisor.logFailure = s.logFailure + supervisor.logBackoff = s.logBackoff + } + if s.state == notRunning { id := s.serviceCounter s.serviceCounter++ @@ -492,12 +513,12 @@ func (s *Supervisor) handleFailedService(id serviceID, err interface{}, stacktra if monitored { if s.state == normal { s.runService(failedService, id) - s.logFailure(failedService, s.failures, s.failureThreshold, true, err, stacktrace) + s.logFailure(s, failedService, s.failures, s.failureThreshold, true, err, stacktrace) } else { // FIXME: When restarting, check that the service still // exists (it may have been stopped in the meantime) s.restartQueue = append(s.restartQueue, id) - s.logFailure(failedService, s.failures, s.failureThreshold, false, err, stacktrace) + s.logFailure(s, failedService, s.failures, s.failureThreshold, false, err, stacktrace) } } } @@ -536,7 +557,7 @@ func (s *Supervisor) removeService(id serviceID) { case <-successChan: // Life is good! case <-failChan: - s.logBadStop(service) + s.logBadStop(s, service) } }() } diff --git a/Godeps/_workspace/src/github.com/thejerf/suture/suture_test.go b/Godeps/_workspace/src/github.com/thejerf/suture/suture_test.go index b61bd4fb7..138198f32 100644 --- a/Godeps/_workspace/src/github.com/thejerf/suture/suture_test.go +++ b/Godeps/_workspace/src/github.com/thejerf/suture/suture_test.go @@ -77,7 +77,7 @@ func TestFailures(t *testing.T) { // to avoid deadlocks during shutdown, we have to not try to send // things out on channels while we're shutting down (this undoes the // logFailure overide about 25 lines down) - s.logFailure = func(Service, float64, float64, bool, interface{}, []byte) {} + s.logFailure = func(*Supervisor, Service, float64, float64, bool, interface{}, []byte) {} s.Stop() }() s.sync() @@ -102,7 +102,7 @@ func TestFailures(t *testing.T) { failNotify := make(chan bool) // use this to synchronize on here - s.logFailure = func(s Service, cf float64, ft float64, r bool, error interface{}, stacktrace []byte) { + s.logFailure = func(supervisor *Supervisor, s Service, cf float64, ft float64, r bool, error interface{}, stacktrace []byte) { failNotify <- r } @@ -276,8 +276,8 @@ func TestDefaultLogging(t *testing.T) { serviceName(&BarelyService{}) - s.logBadStop(service) - s.logFailure(service, 1, 1, true, errors.New("test error"), []byte{}) + s.logBadStop(s, service) + s.logFailure(s, service, 1, 1, true, errors.New("test error"), []byte{}) s.Stop() } @@ -289,9 +289,17 @@ func TestNestedSupervisors(t *testing.T) { super2 := NewSimple("Nested5") service := NewService("Service5") + super2.logBadStop = func(*Supervisor, Service) { + panic("Failed to copy logBadStop") + } + super1.Add(super2) super2.Add(service) + // test the functions got copied from super1; if this panics, it didn't + // get copied + super2.logBadStop(super2, service) + go super1.Serve() super1.sync() @@ -340,7 +348,7 @@ func TestStoppingStillWorksWithHungServices(t *testing.T) { return resumeChan } failNotify := make(chan struct{}) - s.logBadStop = func(s Service) { + s.logBadStop = func(supervisor *Supervisor, s Service) { failNotify <- struct{}{} } @@ -438,7 +446,7 @@ func TestFailingSupervisors(t *testing.T) { } failNotify := make(chan string) // use this to synchronize on here - s1.logFailure = func(s Service, cf float64, ft float64, r bool, error interface{}, stacktrace []byte) { + s1.logFailure = func(supervisor *Supervisor, s Service, cf float64, ft float64, r bool, error interface{}, stacktrace []byte) { failNotify <- fmt.Sprintf("%s", s) } diff --git a/cmd/syncthing/main.go b/cmd/syncthing/main.go index 7f513190f..6521ca8e3 100644 --- a/cmd/syncthing/main.go +++ b/cmd/syncthing/main.go @@ -425,7 +425,9 @@ func upgradeViaRest() error { func syncthingMain() { // Create a main service manager. We'll add things to this as we go along. - // We want any logging it does to go through our log system. + // We want any logging it does to go through our log system. Other + // suture.Supervisor:s that are Add()ed to mainSvc will inherit this log + // function. mainSvc := suture.New("main", suture.Spec{ Log: func(line string) { if debugSuture { diff --git a/internal/beacon/broadcast.go b/internal/beacon/broadcast.go index 5ab2b7f58..d75ad3311 100644 --- a/internal/beacon/broadcast.go +++ b/internal/beacon/broadcast.go @@ -29,12 +29,6 @@ func NewBroadcast(port int) *Broadcast { // a while to get solved... FailureThreshold: 2, FailureBackoff: 60 * time.Second, - // Only log restarts in debug mode. - Log: func(line string) { - if debug { - l.Debugln(line) - } - }, }), port: port, inbox: make(chan []byte), diff --git a/internal/model/model.go b/internal/model/model.go index af24da5c8..7f87c51cf 100644 --- a/internal/model/model.go +++ b/internal/model/model.go @@ -107,13 +107,7 @@ var ( // for file data without altering the local folder in any way. func NewModel(cfg *config.Wrapper, id protocol.DeviceID, deviceName, clientName, clientVersion string, ldb *leveldb.DB) *Model { m := &Model{ - Supervisor: suture.New("model", suture.Spec{ - Log: func(line string) { - if debug { - l.Debugln(line) - } - }, - }), + Supervisor: suture.NewSimple("model"), cfg: cfg, db: ldb, finder: db.NewBlockFinder(ldb, cfg),