Golang HTTP status checker

Question

I'm writing my first somewhat production ready (or not) Go program and could do with some feedback from someone more experienced with go.

The code reads a list of URLs from a JSON file and then makes a GET request to each URL in a loop to check that they are returning a 200 OK response. I have attempted to do this concurrently using go routines but I'm concerned about my synchronization step.

monitor.go

package main

import (
    "encoding/json"
    "fmt"
    "github.com/bradfitz/gomemcache/memcache"
    "os"
    "time"
)

// Configuration encapsulates the result of reading the JSON configuration
// file.
type Configuration struct {
    URLs      []string
    Memcached string
}

// loadConfig loads a configuration file in JSON format and returns a
// Configuration instance.
func loadConfig(path string) (Configuration, error) {
    file, _ := os.Open(path)
    defer file.Close()

    decoder := json.NewDecoder(file)
    configuration := Configuration{}
    err := decoder.Decode(&configuration)

    return configuration, err
}

func main() {
    type Result struct {
        url    string
        status Status
    }

    rc := make(chan Result)

    configuration, err := loadConfig("config.json")

    if err != nil {
        fmt.Println("Error :", err)
        return
    }

    sites := make(map[string]*Site, len(configuration.URLs))

    for _, url := range configuration.URLs {
        sites[url] = &Site{url, UNCHECKED}
    }

    mc := memcache.New(configuration.Memcached)

    sites_output := make(map[string]bool)

    for {
        for _, site := range sites {
            go func(site *Site, rc chan Result) {
                status, _ := site.Status()
                rc <- Result{site.url, status}
            }(site, rc)
        }

        for i := 0; i < len(sites); i++ {
            res := <-rc
            site := sites[res.url]
            if site.last_status != res.status {
                sites[res.url].last_status = res.status
            }
        }

        for k, v := range sites {
            sites_output[k] = v.last_status == 2
        }

        site_json, err := json.Marshal(sites_output)

        if err != nil {
            panic(err)
        }
        mc.Set(&memcache.Item{
            Key:   "mission_control.sites",
            Value: site_json,
        })
        time.Sleep(time.Second * 5)
    }
}

site.go

package main

import (
    "net/http"
)

type Status int

const (
    UNCHECKED Status = iota
    DOWN
    UP
)

// The Site struct encapsulates the details about the site being monitored.
type Site struct {
    url         string
    last_status Status
}

// Site.Status makes a GET request to a given URL and checks whether or not the
// resulting status code is 200.
func (s Site) Status() (Status, error) {
    resp, err := http.Get(s.url)
    status := s.last_status

    if (err == nil) && (resp.StatusCode == 200) {
        status = UP
    } else {
        status = DOWN
    }

    return status, err
}

Answer 1

Style

• Use CamelCase only. last_status -> lastStatus, site_json -> siteJSON, etc.

• Even in constants. UP -> Up or StatusUp, etc.

• Imports are usually split into groups by source, so separate memcache from stdlib with a newline.

• Stuttering. E.g. configuration := Configuration{} could be c := Configuration{}.

• People would search for main() in main.go. So I would rename monitor.go to main.go.

Code

file, _ := os.Open(path)

I see you skip error handling in some places. I'm not sure whether this is just a brief version for this site, or an actual code, but this is a very bad practice.

v.last_status == 2

if (err == nil) && (resp.StatusCode == 200) {

Avoid magic numbers. Use Up/StatusUp and http.StatusOK respectively.

for i := 0; i < len(sites); i++ {

I'd use range. In Go 1.4 it'll be legal to write for range sites as well.

site := sites[res.url]
if site.last_status != res.status {
    sites[res.url].last_status = res.status
}

You index the map twice and make an unnecessary comparison. Wouldn't sites[res.url] = res.status do?

Architecture

If you can change the output JSON format, I'd suggest saving the status code returned by http.Get rather than a simple bool. That will give you more info on why the site is down (4XX and 5XX error) or that the site have moved (3XX).

I have attempted to do this concurrently using go routines but I'm concerned about my synchronization step.

Your synchronisation seems mostly fine to me. (Don't forget to test with -race though). An alternative would be to use sync.WaitGroup and a map with a sync.Mutex.