Worked review 2: worker pool processing files

ProcessAll reads a batch of files with a small pool of worker goroutines and returns their sizes. It compiles, go vet says nothing, and it runs — but it never processes all the files, sometimes leaks goroutines, and (under -race) has a real data race the small demo below happens not to crash on. It has 5 real bugs.

The program

Run ▸ Open in Playground

package main

import (
  "fmt"
  "os"
  "runtime"
  "time"
)

type Result struct {
  Path string
  Size int
}

func processFile(path string, delay time.Duration) (Result, error) {
  time.Sleep(delay) // simulates variable I/O time per file
  data, err := os.ReadFile(path)
  if err != nil {
    return Result{}, err
  }
  return Result{Path: path, Size: len(data)}, nil
}

func ProcessAll(paths []string, delays map[string]time.Duration, numWorkers int) []Result {
  jobs := make(chan string)
  var results []Result

  for i := 0; i < numWorkers; i++ {
    go func() {
      for p := range jobs {
        r, err := processFile(p, delays[p])
        if err == nil {
          results = append(results, r)
        }
      }
    }()
  }

  for _, p := range paths {
    jobs <- p
  }

  time.Sleep(150 * time.Millisecond) // "wait" for workers to finish
  return results
}

func main() {
  dir, _ := os.MkdirTemp("", "wr2")
  defer func() { _ = os.RemoveAll(dir) }()

  names := []string{"a.txt", "bb.txt", "ccc.txt", "slow.txt"}
  delays := map[string]time.Duration{}
  var paths []string
  for i, name := range names {
    p := dir + "/" + name
    _ = os.WriteFile(p, make([]byte, i+1), 0o644)
    paths = append(paths, p)
    delays[p] = 20 * time.Millisecond
  }
  delays[dir+"/slow.txt"] = 400 * time.Millisecond // slower than ProcessAll's fixed wait
  paths = append(paths, dir+"/missing.txt")

  before := runtime.NumGoroutine()
  results := ProcessAll(paths, delays, 2)
  after := runtime.NumGoroutine()

  fmt.Println("processed", len(results), "of", len(paths), "files")
  for _, r := range results {
    fmt.Printf("  %s: %d bytes\n", r.Path[len(dir)+1:], r.Size)
  }
  fmt.Println("goroutines before:", before, "goroutines still running after:", after)
}

Find the bugs

missing.txt doesn’t exist and slow.txt is deliberately slow — but the output shows more than 2 files missing from a batch of 5, and the goroutine count doesn’t return to where it started. Try to name at least three of the five bugs before opening the answer.

Reveal the 5 bugs

1. ProcessAll “waits” for the workers with a fixed time.Sleep(150 * time.Millisecond) instead of a real synchronization primitive. There is no sync.WaitGroup, no done channel — just a guess at how long the work should take. slow.txt is deliberately given a 400ms delay, well past the 150ms guess, so it is silently missing from every run’s results — not reported as an error, just absent, as if it never existed.
2. The jobs channel is never closed. After all paths are sent, nothing tells the worker goroutines to stop; each is parked forever in for p := range jobs, waiting for work that will never arrive. Real output: goroutines before: 1 goroutines still running after: 3 — the 2 worker goroutines never exit.
3. results is a plain []Result appended from multiple goroutines with no mutex. This is a genuine data race — benign-looking in this small run (go run without -race usually completes without crashing), but real under go run -race:

   WARNING: DATA RACE
   Write at 0x00c0001ca000 by goroutine 8:
     main.ProcessAll.func1()
   Previous read at 0x00c0001ca000 by goroutine 7:
     main.ProcessAll.func1()

4. missing.txt’s read error is silently dropped. if err == nil { results = append(...) } means any failure just vanishes — the caller cannot tell “file failed” apart from “file was never scheduled.”
5. The result order is never guaranteed, but nothing accounts for that. Three consecutive real runs of this exact program produced 2 of 5, 3 of 5, and 3 of 5 — a different subset and a different order each time, purely from goroutine scheduling timing. Code that assumed results[0] corresponds to paths[0] would be silently wrong.

The corrected program

Run ▸ Open in Playground

package main

import (
  "fmt"
  "os"
  "runtime"
  "sort"
  "sync"
  "time"
)

type Result struct {
  Path string
  Size int
  Err  error
}

func processFile(path string, delay time.Duration) (Result, error) {
  time.Sleep(delay) // simulates variable I/O time per file
  data, err := os.ReadFile(path)
  if err != nil {
    return Result{}, err
  }
  return Result{Path: path, Size: len(data)}, nil
}

func ProcessAll(paths []string, delays map[string]time.Duration, numWorkers int) []Result {
  jobs := make(chan string)
  resultsCh := make(chan Result)
  var wg sync.WaitGroup // fixed: real completion signal instead of a fixed Sleep

  for i := 0; i < numWorkers; i++ {
    wg.Add(1)
    go func() {
      defer wg.Done()
      for p := range jobs {
        r, err := processFile(p, delays[p])
        if err != nil {
          r = Result{Path: p, Err: err} // fixed: report the failure, don't drop it
        }
        resultsCh <- r
      }
    }()
  }

  go func() {
    for _, p := range paths {
      jobs <- p
    }
    close(jobs) // fixed: workers can now see the channel drain and return
  }()
  go func() {
    wg.Wait()
    close(resultsCh)
  }()

  var results []Result
  for r := range resultsCh { // fixed: single collector goroutine — no shared-slice race
    results = append(results, r)
  }
  sort.Slice(results, func(i, j int) bool { return results[i].Path < results[j].Path }) // fixed: deterministic order
  return results
}

func main() {
  dir, _ := os.MkdirTemp("", "wr2fixed")
  defer func() { _ = os.RemoveAll(dir) }()

  names := []string{"a.txt", "bb.txt", "ccc.txt", "slow.txt"}
  delays := map[string]time.Duration{}
  var paths []string
  for i, name := range names {
    p := dir + "/" + name
    _ = os.WriteFile(p, make([]byte, i+1), 0o644)
    paths = append(paths, p)
    delays[p] = 20 * time.Millisecond
  }
  delays[dir+"/slow.txt"] = 400 * time.Millisecond
  paths = append(paths, dir+"/missing.txt")

  before := runtime.NumGoroutine()
  results := ProcessAll(paths, delays, 2)
  after := runtime.NumGoroutine()

  fmt.Println("processed", len(results), "of", len(paths), "files")
  for _, r := range results {
    name := r.Path[len(dir)+1:]
    if r.Err != nil {
      fmt.Printf("  %s: error: %v\n", name, r.Err)
      continue
    }
    fmt.Printf("  %s: %d bytes\n", name, r.Size)
  }
  fmt.Println("goroutines before:", before, "goroutines still running after:", after)
}

Run the corrected version and it reports processed 5 of 5 files every time, in the same sorted order every time, missing.txt shows up as error: open .../missing.txt: no such file or directory instead of silently vanishing, and goroutines before: 1 goroutines still running after: 1 — the pool actually shuts down. go run -race on this version reports no races, and golangci-lint run reports zero issues.

🟦 A fixed Sleep is never a substitute for a real wait

Bug 1 is the one to internalize: any time.Sleep(N) standing in for “wait until the goroutines are done” is a guess that will eventually be wrong under real load or a slow disk. Use sync.WaitGroup, a done channel, or errgroup.Group — see Goroutines and The sync package.

Three separate runs of the buggy program reported '2 of 5', '3 of 5', and '3 of 5' files processed. What does this variation, by itself, tell you about the bug?

Nothing — the program has a compile error that only sometimes triggers At least one bug depends on goroutine scheduling timing rather than being a fixed, deterministic outcome The files themselves are corrupted os.ReadFile is nondeterministic by design

After ProcessAll returns, runtime.NumGoroutine() shows 2 more goroutines running than before the call. What are those goroutines doing?

They already finished and are being garbage collected They are blocked forever inside for p := range jobs, because jobs is never closed They are retrying failed file reads They are duplicates created by the race detector

Why does go run on the buggy version typically complete without crashing, even though results = append(results, r) is a genuine data race?

It is not actually a race — the compiler would reject it if it were The race detector is always on by default in go run Without the -race flag, Go does not instrument memory accesses to detect concurrent access — the race exists in the program regardless of whether that specific run happens to crash or corrupt visibly Maps and slices in Go are always safe for concurrent access

The fixed version uses a resultsCh channel plus a single collector goroutine (for r := range resultsCh { results = append(...) }) instead of a mutex around the shared slice. Why does this avoid the race without any explicit lock?

Channels are just mutexes with a different name Only one goroutine (the collector) ever touches results, so there is no concurrent access to synchronize — the channel serializes the values into that single goroutine resultsCh automatically deduplicates concurrent writes It doesn't avoid the race — the fixed version still needs a mutex

Submit

Mark as complete