From 879558b9ee9c915fce79d335c4bb6a776acfc51e Mon Sep 17 00:00:00 2001 From: Abiola Ibrahim Date: Tue, 26 May 2015 20:18:19 +0100 Subject: [PATCH 1/2] Git: code refactor. replace Sleep with Ticker --- config/setup/git_test.go | 2 +- middleware/git/git.go | 5 +- middleware/git/service.go | 111 +++++++++++++-------------------- middleware/git/service_test.go | 34 +++++----- 4 files changed, 64 insertions(+), 88 deletions(-) diff --git a/config/setup/git_test.go b/config/setup/git_test.go index cec0fe9b..3a6574b7 100644 --- a/config/setup/git_test.go +++ b/config/setup/git_test.go @@ -87,7 +87,7 @@ No new changes.` } // stop background thread monitor - git.Monitor.StopAndWait(repo.URL, 1) + git.Services.Stop(repo.URL, 1) } diff --git a/middleware/git/git.go b/middleware/git/git.go index 9cfa6565..bff042f1 100644 --- a/middleware/git/git.go +++ b/middleware/git/git.go @@ -33,8 +33,9 @@ var initMutex = sync.Mutex{} // Logger is used to log errors; if nil, the default log.Logger is used. var Logger *log.Logger -// Monitor listens for halt signal to stop repositories from auto pulling. -var Monitor = &monitor{} +// Services holds all git pulling services and provides the function to +// stop them. +var Services = &services{} // logger is an helper function to retrieve the available logger func logger() *log.Logger { diff --git a/middleware/git/service.go b/middleware/git/service.go index dacd1472..55a0956f 100644 --- a/middleware/git/service.go +++ b/middleware/git/service.go @@ -5,106 +5,81 @@ import ( "time" ) -// RepoService is the repository service that runs in background and -// periodic pull from the repository. -type RepoService struct { +// repoService is the service that runs in background and periodically +// pull from the repository. +type repoService struct { repo *Repo + ticker *time.Ticker // ticker to tick at intervals running bool // whether service is running. halt chan struct{} // channel to notify service to halt and stop pulling. - exit chan struct{} // channel to notify on exit. } -// Start starts a new RepoService in background and adds it to monitor. +// Start starts a new background service to pull periodically. func Start(repo *Repo) { - service := &RepoService{ + service := &repoService{ repo, + time.NewTicker(repo.Interval), true, make(chan struct{}), - make(chan struct{}), } - - // start service - go func(s *RepoService) { + go func(s *repoService) { for { - // if service is halted - if !s.running { - // notify exit channel - service.exit <- struct{}{} - break - } - time.Sleep(repo.Interval) - - err := repo.Pull() - if err != nil { - logger().Println(err) + select { + case <-s.ticker.C: + err := repo.Pull() + if err != nil { + logger().Println(err) + } + case <-s.halt: + s.ticker.Stop() + return } } }(service) - // add to monitor to enable halting - Monitor.add(service) + // add to services to make it stoppable + Services.add(service) } -// monitor monitors running services (RepoService) -// and can halt them. -type monitor struct { - services []*RepoService +// services stores all repoServices +type services struct { + services []*repoService sync.Mutex } -// add adds a new service to the monitor. -func (m *monitor) add(service *RepoService) { - m.Lock() - defer m.Unlock() +// add adds a new service to list of services. +func (s *services) add(r *repoService) { + s.Lock() + defer s.Unlock() - m.services = append(m.services, service) - - // start a goroutine to listen for halt signal - service.running = true - go func(r *RepoService) { - <-r.halt - r.running = false - }(service) + s.services = append(s.services, r) } -// Stop stops at most `limit` currently running services that is pulling from git repo at -// repoURL. It returns list of exit channels for the services. A wait for message on the -// channels guarantees exit. If limit is less than zero, it is ignored. +// Stop stops at most `limit` running services pulling from git repo at +// repoURL. It waits until the service is terminated before returning. +// If limit is less than zero, it is ignored. // TODO find better ways to identify repos -func (m *monitor) Stop(repoURL string, limit int) []chan struct{} { - m.Lock() - defer m.Unlock() +func (s *services) Stop(repoURL string, limit int) { + s.Lock() + defer s.Unlock() - var chans []chan struct{} - - // locate services - for i, j := 0, 0; i < len(m.services) && ((limit >= 0 && j < limit) || limit < 0); i++ { - s := m.services[i] - if s.repo.URL == repoURL { + // locate repos + for i, j := 0, 0; i < len(s.services) && ((limit >= 0 && j < limit) || limit < 0); i++ { + service := s.services[i] + if service.repo.URL == repoURL { // send halt signal - s.halt <- struct{}{} - chans = append(chans, s.exit) + service.halt <- struct{}{} + s.services[i] = nil j++ - m.services[i] = nil } } - // remove them from services list - services := m.services[:0] - for _, s := range m.services { + // remove them from repos list + services := s.services[:0] + for _, s := range s.services { if s != nil { services = append(services, s) } } - m.services = services - return chans -} - -// StopAndWait is similar to stop but it waits for the services to terminate before -// returning. -func (m *monitor) StopAndWait(repoUrl string, limit int) { - chans := m.Stop(repoUrl, limit) - for _, c := range chans { - <-c - } + s.services = services } diff --git a/middleware/git/service_test.go b/middleware/git/service_test.go index 114a5893..4041c3fb 100644 --- a/middleware/git/service_test.go +++ b/middleware/git/service_test.go @@ -16,45 +16,45 @@ func Test(t *testing.T) { repo := &Repo{URL: "git@github.com", Interval: time.Second} Start(repo) - if len(Monitor.services) != 1 { - t.Errorf("Expected 1 service, found %v", len(Monitor.services)) + if len(Services.services) != 1 { + t.Errorf("Expected 1 service, found %v", len(Services.services)) } - Monitor.StopAndWait(repo.URL, 1) - if len(Monitor.services) != 0 { - t.Errorf("Expected 1 service, found %v", len(Monitor.services)) + Services.Stop(repo.URL, 1) + if len(Services.services) != 0 { + t.Errorf("Expected 1 service, found %v", len(Services.services)) } repos := make([]*Repo, 5) for i := 0; i < 5; i++ { repos[i] = &Repo{URL: fmt.Sprintf("test%v", i), Interval: time.Second * 2} Start(repos[i]) - if len(Monitor.services) != i+1 { - t.Errorf("Expected %v service(s), found %v", i+1, len(Monitor.services)) + if len(Services.services) != i+1 { + t.Errorf("Expected %v service(s), found %v", i+1, len(Services.services)) } } time.Sleep(time.Second * 5) - Monitor.StopAndWait(repos[0].URL, 1) - if len(Monitor.services) != 4 { - t.Errorf("Expected %v service(s), found %v", 4, len(Monitor.services)) + Services.Stop(repos[0].URL, 1) + if len(Services.services) != 4 { + t.Errorf("Expected %v service(s), found %v", 4, len(Services.services)) } repo = &Repo{URL: "git@github.com", Interval: time.Second} Start(repo) - if len(Monitor.services) != 5 { - t.Errorf("Expected %v service(s), found %v", 5, len(Monitor.services)) + if len(Services.services) != 5 { + t.Errorf("Expected %v service(s), found %v", 5, len(Services.services)) } repo = &Repo{URL: "git@github.com", Interval: time.Second * 2} Start(repo) - if len(Monitor.services) != 6 { - t.Errorf("Expected %v service(s), found %v", 6, len(Monitor.services)) + if len(Services.services) != 6 { + t.Errorf("Expected %v service(s), found %v", 6, len(Services.services)) } time.Sleep(time.Second * 5) - Monitor.StopAndWait(repo.URL, -1) - if len(Monitor.services) != 4 { - t.Errorf("Expected %v service(s), found %v", 4, len(Monitor.services)) + Services.Stop(repo.URL, -1) + if len(Services.services) != 4 { + t.Errorf("Expected %v service(s), found %v", 4, len(Services.services)) } } From 2013838bfdfb48bb07b6507cd3b6683859575753 Mon Sep 17 00:00:00 2001 From: Abiola Ibrahim Date: Thu, 28 May 2015 08:21:10 +0100 Subject: [PATCH 2/2] Git: mock time functions in tests. --- config/setup/git_test.go | 4 +-- middleware/git/git.go | 2 +- middleware/git/git_test.go | 2 +- middleware/git/gitos/gitos.go | 43 +++++++++++++++++++++++++++++++ middleware/git/gittest/gittest.go | 33 ++++++++++++++++++++++-- middleware/git/service.go | 15 +++++------ middleware/git/service_test.go | 4 +-- 7 files changed, 87 insertions(+), 16 deletions(-) diff --git a/config/setup/git_test.go b/config/setup/git_test.go index 3a6574b7..5c49c5eb 100644 --- a/config/setup/git_test.go +++ b/config/setup/git_test.go @@ -57,14 +57,14 @@ func TestIntervals(t *testing.T) { check(t, err) // wait for first background pull - time.Sleep(time.Millisecond * 100) + gittest.Sleep(time.Millisecond * 100) // switch logger to test file logFile := gittest.Open("file") git.Logger = log.New(logFile, "", 0) // sleep for the interval - time.Sleep(repo.Interval) + gittest.Sleep(repo.Interval) // get log output out, err := ioutil.ReadAll(logFile) diff --git a/middleware/git/git.go b/middleware/git/git.go index bff042f1..6695dab1 100644 --- a/middleware/git/git.go +++ b/middleware/git/git.go @@ -68,7 +68,7 @@ func (r *Repo) Pull() error { defer r.Unlock() // prevent a pull if the last one was less than 5 seconds ago - if time.Since(r.lastPull) < 5*time.Second { + if gos.TimeSince(r.lastPull) < 5*time.Second { return nil } diff --git a/middleware/git/git_test.go b/middleware/git/git_test.go index 23cd4969..695e3b5b 100644 --- a/middleware/git/git_test.go +++ b/middleware/git/git_test.go @@ -160,7 +160,7 @@ Command echo Hello successful. before := r.repo.lastPull - time.Sleep(r.repo.Interval) + gittest.Sleep(r.repo.Interval) err = r.repo.Pull() after := r.repo.lastPull diff --git a/middleware/git/gitos/gitos.go b/middleware/git/gitos/gitos.go index 5da3f4fa..a38874c0 100644 --- a/middleware/git/gitos/gitos.go +++ b/middleware/git/gitos/gitos.go @@ -5,6 +5,7 @@ import ( "io/ioutil" "os" "os/exec" + "time" ) // File is an abstraction for file (os.File). @@ -114,6 +115,33 @@ type OS interface { // beginning with prefix, opens the file for reading and writing, and // returns the resulting File. TempFile(string, string) (File, error) + + // Sleep pauses the current goroutine for at least the duration d. A + // negative or zero duration causes Sleep to return immediately. + Sleep(time.Duration) + + // NewTicker returns a new Ticker containing a channel that will send the + // time with a period specified by the argument. + NewTicker(time.Duration) Ticker + + // TimeSince returns the time elapsed since the argument. + TimeSince(time.Time) time.Duration +} + +// Ticker is an abstraction for Ticker (time.Ticker) +type Ticker interface { + C() <-chan time.Time + Stop() +} + +// GitTicker is the implementation of Ticker for git. +type GitTicker struct { + *time.Ticker +} + +// C returns the channel on which the ticks are delivered.s +func (g *GitTicker) C() <-chan time.Time { + return g.Ticker.C } // GitOS is the implementation of OS for git. @@ -158,3 +186,18 @@ func (g GitOS) ReadDir(dirname string) ([]os.FileInfo, error) { func (g GitOS) Command(name string, args ...string) Cmd { return &gitCmd{exec.Command(name, args...)} } + +// Sleep calls time.Sleep. +func (g GitOS) Sleep(d time.Duration) { + time.Sleep(d) +} + +// New Ticker calls time.NewTicker. +func (g GitOS) NewTicker(d time.Duration) Ticker { + return &GitTicker{time.NewTicker(d)} +} + +// TimeSince calls time.Since +func (g GitOS) TimeSince(t time.Time) time.Duration { + return time.Since(t) +} diff --git a/middleware/git/gittest/gittest.go b/middleware/git/gittest/gittest.go index 9e9a116f..a275b281 100644 --- a/middleware/git/gittest/gittest.go +++ b/middleware/git/gittest/gittest.go @@ -19,6 +19,9 @@ var CmdOutput = "success" // TempFileName is the name of any file returned by mocked gitos.OS's TempFile(). var TempFileName = "tempfile" +// TimeSpeed is how faster the mocked gitos.Ticker and gitos.Sleep should run. +var TimeSpeed = 5 + // dirs mocks a fake git dir if filename is "gitdir". var dirs = map[string][]os.FileInfo{ "gitdir": { @@ -31,6 +34,11 @@ func Open(name string) gitos.File { return &fakeFile{name: name} } +// Sleep calls fake time.Sleep +func Sleep(d time.Duration) { + FakeOS.Sleep(d) +} + // fakeFile is a mock gitos.File. type fakeFile struct { name string @@ -70,7 +78,7 @@ func (f *fakeFile) Write(b []byte) (int, error) { return len(b), nil } -// fakeCmd is a mock git.Cmd. +// fakeCmd is a mock gitos.Cmd. type fakeCmd struct{} func (f fakeCmd) Run() error { @@ -128,7 +136,16 @@ func (f fakeInfo) Sys() interface{} { return nil } -// fakeOS is a mock git.OS. +// fakeTicker is a mock gitos.Ticker +type fakeTicker struct { + *time.Ticker +} + +func (f fakeTicker) C() <-chan time.Time { + return f.Ticker.C +} + +// fakeOS is a mock gitos.OS. type fakeOS struct{} func (f fakeOS) Mkdir(name string, perm os.FileMode) error { @@ -165,3 +182,15 @@ func (f fakeOS) ReadDir(dirname string) ([]os.FileInfo, error) { func (f fakeOS) Command(name string, args ...string) gitos.Cmd { return fakeCmd{} } + +func (f fakeOS) Sleep(d time.Duration) { + time.Sleep(d / time.Duration(TimeSpeed)) +} + +func (f fakeOS) NewTicker(d time.Duration) gitos.Ticker { + return &fakeTicker{time.NewTicker(d / time.Duration(TimeSpeed))} +} + +func (f fakeOS) TimeSince(t time.Time) time.Duration { + return time.Since(t) * time.Duration(TimeSpeed) +} diff --git a/middleware/git/service.go b/middleware/git/service.go index 55a0956f..224f37a6 100644 --- a/middleware/git/service.go +++ b/middleware/git/service.go @@ -2,30 +2,29 @@ package git import ( "sync" - "time" + + "github.com/mholt/caddy/middleware/git/gitos" ) // repoService is the service that runs in background and periodically // pull from the repository. type repoService struct { - repo *Repo - ticker *time.Ticker // ticker to tick at intervals - running bool // whether service is running. - halt chan struct{} // channel to notify service to halt and stop pulling. + repo *Repo + ticker gitos.Ticker // ticker to tick at intervals + halt chan struct{} // channel to notify service to halt and stop pulling. } // Start starts a new background service to pull periodically. func Start(repo *Repo) { service := &repoService{ repo, - time.NewTicker(repo.Interval), - true, + gos.NewTicker(repo.Interval), make(chan struct{}), } go func(s *repoService) { for { select { - case <-s.ticker.C: + case <-s.ticker.C(): err := repo.Pull() if err != nil { logger().Println(err) diff --git a/middleware/git/service_test.go b/middleware/git/service_test.go index 4041c3fb..5312333a 100644 --- a/middleware/git/service_test.go +++ b/middleware/git/service_test.go @@ -34,7 +34,7 @@ func Test(t *testing.T) { } } - time.Sleep(time.Second * 5) + gos.Sleep(time.Second * 5) Services.Stop(repos[0].URL, 1) if len(Services.services) != 4 { t.Errorf("Expected %v service(s), found %v", 4, len(Services.services)) @@ -52,7 +52,7 @@ func Test(t *testing.T) { t.Errorf("Expected %v service(s), found %v", 6, len(Services.services)) } - time.Sleep(time.Second * 5) + gos.Sleep(time.Second * 5) Services.Stop(repo.URL, -1) if len(Services.services) != 4 { t.Errorf("Expected %v service(s), found %v", 4, len(Services.services))