Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

os: data race between Process.Wait and Kill #9382

Closed
pmezard opened this issue Dec 18, 2014 · 1 comment
Closed

os: data race between Process.Wait and Kill #9382

pmezard opened this issue Dec 18, 2014 · 1 comment
Milestone

Comments

@pmezard
Copy link
Contributor

pmezard commented Dec 18, 2014

go version go1.4 windows/amd64

Looks like there is a data race between os.Process.Wait() and os.Process.Kill(). The documentation does not state these operations can be run concurrently, but if they cannot, can you suggest a way to implement some kind of timeout on started process? Like "kill it if it runs more than N seconds".

The following code triggers the issue:

package main

import (
    "fmt"
    "os"
    "os/exec"
    "strconv"
    "sync"
    "time"
)

func spawnAndKill(exePath string, counter int) error {
    cmd := exec.Command(exePath, fmt.Sprintf("%d", counter))
    err := cmd.Start()
    if err != nil {
        return err
    }
    go func() {
        time.Sleep(1000 * time.Millisecond)
        cmd.Process.Kill()
    }()
    cmd.Wait()
    return nil
}

func main() {
    args := os.Args[1:]
    if len(args) > 0 {
        delay, err := strconv.ParseInt(args[0], 10, 32)
        if err != nil {
            panic("invalid input: " + args[0])
        }
        time.Sleep(time.Duration(delay) * time.Millisecond)
        return
    }
    exePath := os.Args[0]
    fmt.Printf("exe: %s\n", exePath)

    jobs := &sync.WaitGroup{}
    for j := 0; j < 100; j++ {
        jobs.Add(1)
        go func(id int) {
            defer jobs.Done()
            counter := 0
            for {
                err := spawnAndKill(exePath, counter%2000)
                if err != nil {
                    fmt.Printf("spawn error: %s\n", err)
                    return
                }
                counter += 1
            }
        }(j)
    }
    jobs.Wait()
}

when run with:

go install -race race && GOMAXPROCS=100 GORACE="halt_on_error=1" bin/race.exe

and triggers:

exe: F:\dev\master\sword2\bin\race.exe
==================
WARNING: DATA RACE
Read by goroutine 106:
  os.(*Process).signal()
      c:/go/src/os/exec_windows.go:56 +0x5a
  os.(*Process).Signal()
      c:/go/src/os/doc.go:51 +0x62
  os.(*Process).kill()
      c:/go/src/os/exec_posix.go:53 +0x76
  os.(*Process).Kill()
      c:/go/src/os/doc.go:36 +0x4e
  main.func·001()
      F:/dev/master/sword2/src/race/race.go:20 +0x83

Previous write by goroutine 6:
  os.(*Process).release()
      c:/go/src/os/exec_windows.go:77 +0x2c2
  os.(*Process).Release()
      c:/go/src/os/doc.go:31 +0x4e
  os.(*Process).wait()
      c:/go/src/os/exec_windows.go:42 +0x64c
  os.(*Process).Wait()
      c:/go/src/os/doc.go:45 +0x4e
  os/exec.(*Cmd).Wait()
      c:/go/src/os/exec/exec.go:364 +0x315
  main.spawnAndKill()
      F:/dev/master/sword2/src/race/race.go:22 +0x360
  main.func·002()
      F:/dev/master/sword2/src/race/race.go:46 +0xe1

Goroutine 106 (running) created at:
  main.spawnAndKill()
      F:/dev/master/sword2/src/race/race.go:21 +0x340
  main.func·002()
      F:/dev/master/sword2/src/race/race.go:46 +0xe1

Goroutine 6 (running) created at:
  main.main()
      F:/dev/master/sword2/src/race/race.go:53 +0x539
==================
@ianlancetaylor ianlancetaylor added this to the Go1.5 milestone Dec 18, 2014
@rsc rsc removed the repo-main label Apr 14, 2015
pmezard pushed a commit to pmezard/go that referenced this issue May 10, 2015
Process.handle was accessed without synchronization while wait() and
signal() could be called concurrently.

A first solution was to add a Mutex in Process but it was probably too
invasive given Process.handle is only used on Windows.

This version uses atomic operations to read the handle value. There is
still a race between isDone() and the value of the handle, but it only
leads to slightly incorrect error codes. The caller may get a:

  errors.New("os: process already finished")

instead of:

  syscall.EINVAL

which sounds harmless. I did add accessors for the atomic operations
since they only exist in the Windows version.

Fixes golang#9382
@gopherbot
Copy link

CL https://golang.org/cl/9904 mentions this issue.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants