Golang concurrent writer

Question

I wrote a program that concurrently reads multiple files and makes a request to an API for each line of each file. The JSON response is then parsed and the "available_filters" field is extracted. The program then evaluates if each "filters" field inside "available_filters" contains numbers in its name. Each numeric filter is then written to a single file.

I'm new to go, so my main concern is: am I using concurrently correctly/ to its full potential? How could I improve the handling of my goroutines? (in particular: having two wait groups in main() feels a little wonky to me). And any general tips you could give me.

Thanks!

Here's the code:

package main

import (
    "bufio"
    "encoding/json"
    "flag"
    "fmt"
    "io/ioutil"
    "net/http"
    "os"
    "regexp"
    "strings"
    "sync"
)

var filterBlacklist = map[string]bool{
    "blacklistedName1": true,
    "blacklistedName2": true,
    "blacklistedName3": true,
    "blacklistedName4": true,
}

type AvailableFilters struct {
    Available_filters []Filter
}

type Filter struct {
    ID     string
    Name   string
    Type   string
    Values []Value
}

type Value struct {
    ID      string
    Name    string
    Results int64
}

func (f *Filter) toString() string {
    var ret string = fmt.Sprintf("%s {\n", f.Name)
    for _, value := range f.Values {
        ret += fmt.Sprintf("\t%s\n", value.Name)
    }
    ret += "}"
    return ret
}

func check(e error) {
    if e != nil {
        panic(e)
    }
}

func readFileAndRequest(filepath string, c chan<- string) {
    file, err := os.Open(filepath)
    check(err)
    defer file.Close()
    scanner := bufio.NewScanner(file)

    var wg sync.WaitGroup
    for scanner.Scan() {
        var line string = scanner.Text()
        var url string = fmt.Sprintf("https://apirequestmock?q=%s", line)

        wg.Add(1)
        go func(url string, c chan<- string) {
            writeResponse(url, c)
            wg.Done()
        }(url, c)
    }
    wg.Wait()
}

func writeResponse(url string, c chan<- string) {
    var filters = new(AvailableFilters)
    getJSON(url, &filters)

    var wg sync.WaitGroup
    for _, filter := range filters.Available_filters {
        wg.Add(1)

        go func(filter Filter, c chan<- string) {
            if isNumericFilter(filter) {
                c <- filter.toString()
            }
            wg.Done()
        }(filter, c)
    }

    wg.Wait()
}

func writeToFile(c <-chan string, outputpath string, wg *sync.WaitGroup) {
    output, err := os.Create(outputpath)
    check(err)
    defer output.Close()

    w := bufio.NewWriter(output)

    for v := range c {
        _, err := w.WriteString(v + "\n")
        check(err)
    }
    fmt.Println("Writing output to file ", output.Name())
    check(w.Flush())
    wg.Done()
    return
}

func getJSON(url string, target interface{}) error {
    r, err := http.Get(url)
    check(err)

    defer r.Body.Close()

    body, err := ioutil.ReadAll(r.Body)
    check(err)
    return json.Unmarshal(body, target)
}

func isNumericFilter(filter Filter) bool {
    if filterBlacklist[strings.ToLower(filter.Name)] {
        return false
    }
    var hasNumber bool = true
    var valuesLen int = len(filter.Values)
    for i, v := range filter.Values {
        has, err := regexp.MatchString("^.*[0-9]+.*$", v.Name)
        check(err)
        hasNumber = hasNumber && (has || (valuesLen > 1 && i == (valuesLen-1)))
    }
    return hasNumber
}

func main() {
    path := flag.String("path", ".", "Path to files.")
    outputpath := flag.String("output", "output", "Output filepath")
    flag.Parse()
    fileNames := flag.Args()

    var c chan string = make(chan string)
    var wait sync.WaitGroup
    wait.Add(1)
    go writeToFile(c, *outputpath, &wait)

    var wg sync.WaitGroup
    for _, f := range fileNames {
        wg.Add(1)
        go func(f string, path string, c chan string) {
            var file string = fmt.Sprintf("%s/%s", path, f)
            readFileAndRequest(file, c)

            wg.Done()
        }(f, *path, c)
    }
    wg.Wait()
    close(c)
    wait.Wait()
}

Here's a mock of the an input file:

QUERY1
QUERY2
QUERY3

Here's a mock of the api response:

{
    "field1": "value",
    "field2": "value",
    "available_filters": [
        {
          "id": "id value",
          "name": "name value",
          "type": "text",
          "values": [
            {
              "id": "filter id1",
              "name": "11 GB",
              "results": 1674
            },
            {
              "id": "filter id2",
              "name": "40 GB",
              "results": 333
            },
            {
              "id": "filter id3",
              "name": "3 GB",
              "results": 23
            },
            {
              "id": "filter id3",
              "name": "Other values",
              "results": 23
            },
          ]
        },
        {
          "id": "id value",
          "name": "name value",
          "type": "text",
          "values": [
            {
              "id": "filter id1",
              "name": "23 miles",
              "results": 1674
            },
            {
              "id": "filter id2",
              "name": "1 mile",
              "results": 333
            }
          ]
        }
    ]
}

Answer 1

In the following code snippet you're allocating a go routine to perform a simple calculation.

go func(filter Filter, c chan<- string) {
    if isNumericFilter(filter) {
        c <- filter.toString()
    }
    wg.Done()
}(filter, c)

If I check out the isNumericFilter function I see that there is nothing blocking here.

func isNumericFilter(filter Filter) bool {
    if filterBlacklist[strings.ToLower(filter.Name)] {
        return false
    }
    var hasNumber bool = true
    var valuesLen int = len(filter.Values)
    for i, v := range filter.Values {
        has, err := regexp.MatchString("^.*[0-9]+.*$", v.Name)
        check(err)
        hasNumber = hasNumber && (has || (valuesLen > 1 && i == (valuesLen-1)))
    }
    return hasNumber
}

So I think your allocation of a go routine here is just a waste of resources.

Go routine are supposed to be used in all the case where you have to deal with blocking code, like read a file, access a database or, as in your case access to a remote API.

All cases when your local thread as nothing to do but wait for an answer.

The function isNumericFilter just perform calculation, so the results is your actual go routine will wait while a go routine perform the same task that your actual go routine could perform.

I think your point here is that you could do it concurrently, but my point is that the calculation you want to perform concurrently is not so slow.

Consider that allocating go routine is not free, so you should first try the to do in the same go routine and just after you see that a part is slow and could be performed concurrently, than you should go ahead with new go routines.

Another point that I can see in your code is you don't use a channel for errors.

When you work in concurrency and something went wrong you should handle the message in the same way you handle the data, I mean through the communication between go routines.

Usually an error channel help to handle this.

Another minor point.

I don't like so much the way you trait error checking with a function:

func check(e error) {
    if e != nil {
        panic(e)
    }
}

The name here is very bad. If you found this function inside the code you could not understand that is going to stop your program!

A better name could be: exitOnError()

Answer 2

• AvailableFilters could just be type AvailableFilters []Filter.
• Many variable definitions could just use := instead of repeating the type all over, e.g. ret := fmt.Sprintf....
• I'm hesitant with panic, but if that's how you want to do it. In general I've mostly seen the if err != nil { return err } style instead. In fact in our projects I'd rather not see a single panic outside of testing code, it's really a last resort measure.
• Still regarding error checking, the most important one, flag.Parse() isn't being checked. Also path plus a file name isn't a good idea, that the current working directory is the default is already obvious, so leave out the path and then rely on the shell and/or other caller to specify the paths correctly!
• Empty returns do nothing.

Have to think about the wait groups, kinda agree with having two is a bit weird.