From 97b1c66d4a614b9ca9f781d4b8cdee0366c4f6bb Mon Sep 17 00:00:00 2001 From: Jakob Borg Date: Thu, 14 Jan 2016 11:06:36 +0100 Subject: [PATCH] Improve API/GUI shutdown handling (fixes #2694) This fixes both a race condition where we could assign s.stop from one goroutine and then read it from another without locking, and handles the fact that listener may be nil at shutdown if we've had a bad CommitConfiguration call in the meantime. --- cmd/syncthing/gui.go | 50 +++++++++++++++++++++++++++++++------- cmd/syncthing/gui_test.go | 51 ++++++++++++++++++++++++++++++++++++++- 2 files changed, 91 insertions(+), 10 deletions(-) diff --git a/cmd/syncthing/gui.go b/cmd/syncthing/gui.go index 38b9672e0..6410d15df 100644 --- a/cmd/syncthing/gui.go +++ b/cmd/syncthing/gui.go @@ -57,10 +57,14 @@ type apiService struct { eventSub *events.BufferedSubscription discoverer *discover.CachingMux relayService *relay.Service - listener net.Listener fss *folderSummaryService - stop chan struct{} - systemConfigMut sync.Mutex + systemConfigMut sync.Mutex // serializes posts to /rest/system/config + stop chan struct{} // signals intentional stop + configChanged chan struct{} // signals intentional listener close due to config change + started chan struct{} // signals startup complete, for testing only + + listener net.Listener + listenerMut sync.Mutex guiErrors *logger.Recorder systemLog *logger.Recorder @@ -76,6 +80,9 @@ func newAPIService(id protocol.DeviceID, cfg *config.Wrapper, assetDir string, m discoverer: discoverer, relayService: relayService, systemConfigMut: sync.NewMutex(), + stop: make(chan struct{}), + configChanged: make(chan struct{}), + listenerMut: sync.NewMutex(), guiErrors: errors, systemLog: systemLog, } @@ -146,7 +153,15 @@ func sendJSON(w http.ResponseWriter, jsonObject interface{}) { } func (s *apiService) Serve() { - s.stop = make(chan struct{}) + s.listenerMut.Lock() + listener := s.listener + s.listenerMut.Unlock() + + if listener == nil { + // Not much we can do here other than exit quickly. The supervisor + // will log an error at some point. + return + } // The GET handlers getRestMux := http.NewServeMux() @@ -249,23 +264,37 @@ func (s *apiService) Serve() { defer s.fss.Stop() s.fss.ServeBackground() - l.Infoln("API listening on", s.listener.Addr()) + l.Infoln("API listening on", listener.Addr()) l.Infoln("GUI URL is", guiCfg.URL()) - err := srv.Serve(s.listener) + if s.started != nil { + // only set when run by the tests + close(s.started) + } + err := srv.Serve(listener) // The return could be due to an intentional close. Wait for the stop // signal before returning. IF there is no stop signal within a second, we // assume it was unintentional and log the error before retrying. select { case <-s.stop: + case <-s.configChanged: case <-time.After(time.Second): l.Warnln("API:", err) } } func (s *apiService) Stop() { + s.listenerMut.Lock() + listener := s.listener + s.listenerMut.Unlock() + close(s.stop) - s.listener.Close() + + // listener may be nil here if we've had a config change to a broken + // configuration, in which case we shouldn't try to close it. + if listener != nil { + listener.Close() + } } func (s *apiService) String() string { @@ -285,7 +314,10 @@ func (s *apiService) CommitConfiguration(from, to config.Configuration) bool { // must create a new listener before Serve() starts again. We can't create // a new listener on the same port before the previous listener is closed. // To assist in this little dance the Serve() method will wait for a - // signal on the stop channel after the listener has closed. + // signal on the configChanged channel after the listener has closed. + + s.listenerMut.Lock() + defer s.listenerMut.Unlock() s.listener.Close() @@ -299,7 +331,7 @@ func (s *apiService) CommitConfiguration(from, to config.Configuration) bool { return false } - close(s.stop) + s.configChanged <- struct{}{} return true } diff --git a/cmd/syncthing/gui_test.go b/cmd/syncthing/gui_test.go index f286fc2e4..2abfb0c3c 100644 --- a/cmd/syncthing/gui_test.go +++ b/cmd/syncthing/gui_test.go @@ -6,7 +6,13 @@ package main -import "testing" +import ( + "testing" + + "github.com/syncthing/syncthing/lib/config" + "github.com/syncthing/syncthing/lib/protocol" + "github.com/thejerf/suture" +) func TestCSRFToken(t *testing.T) { t1 := newCsrfToken() @@ -40,3 +46,46 @@ func TestCSRFToken(t *testing.T) { t.Fatal("t3 should have expired by now") } } + +func TestStopAfterBrokenConfig(t *testing.T) { + baseDirs["config"] = "../../test/h1" // to load HTTPS keys + expandLocations() + + cfg := config.Configuration{ + GUI: config.GUIConfiguration{ + RawAddress: "127.0.0.1:0", + RawUseTLS: false, + }, + } + w := config.Wrap("/dev/null", cfg) + + srv, err := newAPIService(protocol.LocalDeviceID, w, "", nil, nil, nil, nil, nil, nil) + if err != nil { + t.Fatal(err) + } + srv.started = make(chan struct{}) + + sup := suture.NewSimple("test") + sup.Add(srv) + sup.ServeBackground() + + <-srv.started + + // Service is now running, listening on a random port on localhost. Now we + // request a config change to a completely invalid listen address. The + // commit will fail and the service will be in a broken state. + + newCfg := config.Configuration{ + GUI: config.GUIConfiguration{ + RawAddress: "totally not a valid address", + RawUseTLS: false, + }, + } + if srv.CommitConfiguration(cfg, newCfg) { + t.Fatal("Config commit should have failed") + } + + // Nonetheless, it should be fine to Stop() it without panic. + + sup.Stop() +}