1

I want to write a worker which does a job say every 1 hour. There has been no crons in our system as of now so don't want to add a cron. Request for suggestion in the followin implementation:

func init(){
     go startWorker()
}

func startWorker(){
     doJob()
     time.Sleep(time.Second * 3600)
}

Is using a sleep a bad idea in a go routine or are there better alternatives to do such stuff. The job of doJob() is to fetch from the DB all the failures that have happened in the last hour and do a retry

doubt
  • 315
  • 3
  • 18
  • 3
    Use a [`Ticker`](https://golang.org/pkg/time/#Ticker), that's what it's made for. – Adrian Jun 02 '20 at 19:11
  • 1
    https://stackoverflow.com/a/32148983/6069012 – Gavin Jun 02 '20 at 19:12
  • Does this answer your question? [Behavior of sleep and select in go](https://stackoverflow.com/questions/32147421/behavior-of-sleep-and-select-in-go) –  Jun 02 '20 at 19:29

1 Answers1

2

There are two issues with using time.Sleep in the way you envision, one major and one minor.

You have no way to terminate the goroutine

The goroutine is stuck in an infinite loop, so unless doJob panics, it will never terminate. You should probably pass it a channel that will be closed when the goroutine needs to terminate:

done := make(chan struct{})
go worker(done)
...
close(done)
...
func worker(done <-chan struct{}){
    for {
        doJob()
        timer := time.NewTimer(time.Hour)
        select {
        case <-timer.C:
            // nothing
        case <-done:
            timer.Stop()
            return
        }
    }
}

Or, better yet, using a ticker:

func worker(done <-chan struct{}){
    ticker := time.NewTicker(time.Hour)
    for {
        doJob()
        select {
        case <-ticker.C:
            // nothing
        case <-done:
           ticker.Stop()
           return
        }
    }
}

It is good style here to use defer:

func worker(done <-chan struct{}){
    ticker := time.NewTicker(time.Hour)
    defer ticker.Stop()
    for {
        doJob()
        select {
        case <-ticker.C:
            // nothing
        case <-done:
           return
        }
    }
}

You're wasting a stack

While a goroutine uses negligible CPU resources, it uses a stack, on the order of a dozen kilobytes or so. That's probably not a problem for you, but if it is, and if your application has a main loop, then you can insert your ticker and invocation of doWork into the main select statement of the main loop.

Juve
  • 10,584
  • 14
  • 63
  • 90
jch
  • 5,382
  • 22
  • 41
  • You can also use `context.Context` instead of an explicit `done` channel. – Tarun Khandelwal Jun 03 '20 at 13:57
  • I have not found `Context` to be particularly useful, but perhaps I'm holding it wrong. – jch Jun 04 '20 at 15:42
  • Not sure how are you using contexts but they're very useful if it used correctly. This particular case is a textbook example of where the context would work great. The function becomes `func worker(ctx context.Context)` and the case becomes `case <-ctx.Done()`. – Tarun Khandelwal Jun 04 '20 at 17:16
  • I understand *how* to use contexts. What I don't understand is what using a context buys you over a naked channel, except in the rare cases where you're actually building a tree of contexts. – jch Jul 10 '20 at 10:51
  • Using nested contexts isn't a rarity though - it is used pretty often. Even if it wasn't, it abstracts some complexity out. There is also no reason to implement the same thing again in your code, especially when you want to use a lib that accepts context. – Tarun Khandelwal Jul 10 '20 at 15:48