Skip to content
This repository has been archived by the owner on Nov 14, 2023. It is now read-only.

Improve signal handling for df timeout #16

Merged
merged 3 commits into from
Aug 31, 2015
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 23 additions & 18 deletions filesystem/filesystem.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,37 +15,42 @@ func getFileSystemInfo() (interface{}, error) {
/* Grab filesystem data from df */
cmd := exec.Command("df", dfOptions...)

outCh := make(chan []byte)
errCh := make(chan error)
outCh := make(chan []byte, 1)
errCh := make(chan error, 1)

var out interface{}
var err error

go func() {
_out, _err := cmd.Output()
if _err != nil {
errCh <- _err
errCh <- fmt.Errorf("df failed to collect filesystem data: %s", _err)
return
}
outCh <- _out
}()

select {
case res := <-outCh:
if res != nil {
out, err = parseDfOutput(string(res))
} else {
out, err = nil, fmt.Errorf("df process timed out and was killed!")
WAIT:
for {
select {
case res := <-outCh:
if res != nil {
out, err = parseDfOutput(string(res))
} else {
out, err = nil, fmt.Errorf("df failed to collect filesystem data")
}
break WAIT
case err = <-errCh:
out = nil
break WAIT
case <-time.After(2 * time.Second):
// Kill the process if it takes too long
if killErr := cmd.Process.Kill(); killErr != nil {
log.Fatal("failed to kill:", killErr)
// Force goroutine to exit
<-outCh
}
}
case err = <-errCh:
out = nil
case <-time.After(2 * time.Second):
// Kill the process if it takes too long
if killErr := cmd.Process.Kill(); killErr != nil {
log.Fatal("failed to kill:", killErr)
}
//Let goroutine exit
<-outCh
}

return out, err
Expand Down
72 changes: 72 additions & 0 deletions filesystem/filesystem_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
package filesystem

import (
"fmt"
"log"
"os/exec"
"reflect"
"testing"
"time"
)

func MockSlowGetFileSystemInfo() (interface{}, error) {
/* Run a command that will definitely time out */
cmd := exec.Command("sleep", "5")

outCh := make(chan []byte, 1)
errCh := make(chan error, 1)

var out interface{}
var err error

go func() {
_out, _err := cmd.Output()
if _err != nil {
errCh <- fmt.Errorf("df failed to collect filesystem data: %s", _err)
return
}
outCh <- _out
}()

WAIT:
for {
select {
case res := <-outCh:
if res != nil {
out, err = parseDfOutput(string(res))
} else {
out, err = nil, fmt.Errorf("df failed to collect filesystem data")
}
break WAIT
case err = <-errCh:
out = nil
break WAIT
case <-time.After(2 * time.Second):
// Kill the process if it takes too long
if killErr := cmd.Process.Kill(); killErr != nil {
log.Fatal("failed to kill:", killErr)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we have to kill the whole process anymore ? Can't we just return the error ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought it made sense to ensure that multiple df calls spawned by gohai weren't running at the same time. The change was prompted by a case where df was taking long and successive gohai calls kept spawning new, time-intensive processes. This will kill df before it returns, and not interrupt the main gohai process

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yeah misread the code. looks good to me!

// Force goroutine to exit
<-outCh
}
}
}

return out, err
}

func TestSlowGetFileSystemInfo(t *testing.T) {
out, err := MockSlowGetFileSystemInfo()
if !reflect.DeepEqual(out, nil) {
t.Fatalf("Failed! out should be nil. Instead it's %s", out)
}
if !reflect.DeepEqual(err, fmt.Errorf("df failed to collect filesystem data: signal: killed")) {
t.Fatalf("Failed! Wrong error: %s", err)
}
}

func TestGetFileSystemInfo(t *testing.T) {
_, err := getFileSystemInfo()
if !reflect.DeepEqual(err, nil) {
t.Fatalf("getFileSystemInfo failed: %s", err)
}
}