patterngoModerate
Golang HTTP status checker
Viewed 0 times
golanghttpstatuschecker
Problem
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
}
}
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
}
}
Solution
Style
-
Use CamelCase only.
-
Even in constants.
-
Imports are usually split into groups by source, so separate
-
Stuttering. E.g.
-
People would search for
Code
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.
Avoid magic numbers. Use
I'd use
You index the map twice and make an unnecessary comparison. Wouldn't
Architecture
If you can change the output JSON format, I'd suggest saving the status code returned by
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
-
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.Code Snippets
file, _ := os.Open(path)v.last_status == 2
if (err == nil) && (resp.StatusCode == 200) {for i := 0; i < len(sites); i++ {site := sites[res.url]
if site.last_status != res.status {
sites[res.url].last_status = res.status
}Context
StackExchange Code Review Q#64254, answer score: 15
Revisions (0)
No revisions yet.