Skip to content

Commit

Permalink
Merge pull request #2394 from adamwg/reap-the-reaper
Browse files Browse the repository at this point in the history
Remove the child process reaper (resolve #1988)
  • Loading branch information
slackpad authored Oct 7, 2016
2 parents 55a00d4 + de4fcf9 commit 24153f6
Show file tree
Hide file tree
Showing 11 changed files with 4 additions and 591 deletions.
9 changes: 0 additions & 9 deletions command/agent/agent.go
Original file line number Diff line number Diff line change
Expand Up @@ -116,14 +116,6 @@ type Agent struct {
// agent methods use this, so use with care and never override
// outside of a unit test.
endpoints map[string]string

// reapLock is used to prevent child process reaping from interfering
// with normal waiting for subprocesses to complete. Any time you exec
// and wait, you should take a read lock on this mutex. Only the reaper
// takes the write lock. This setup prevents us from serializing all the
// child process management with each other, it just serializes them
// with the child process reaper.
reapLock sync.RWMutex
}

// Create is used to create a new Agent. Returns
Expand Down Expand Up @@ -1042,7 +1034,6 @@ func (a *Agent) AddCheck(check *structs.HealthCheck, chkType *CheckType, persist
Interval: chkType.Interval,
Timeout: chkType.Timeout,
Logger: a.logger,
ReapLock: &a.reapLock,
}
monitor.Start()
a.checkMonitors[check.CheckID] = monitor
Expand Down
32 changes: 2 additions & 30 deletions command/agent/command.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ import (
"github.com/hashicorp/consul/lib"
"github.com/hashicorp/consul/watch"
"github.com/hashicorp/go-checkpoint"
"github.com/hashicorp/go-reap"
"github.com/hashicorp/go-syslog"
"github.com/hashicorp/logutils"
scada "github.com/hashicorp/scada-client/scada"
Expand Down Expand Up @@ -771,33 +770,6 @@ func (c *Command) Run(args []string) int {
defer server.Shutdown()
}

// Enable child process reaping
if (config.Reap != nil && *config.Reap) || (config.Reap == nil && os.Getpid() == 1) {
if !reap.IsSupported() {
c.Ui.Error("Child process reaping is not supported on this platform (set reap=false)")
return 1
} else {
logger := c.agent.logger
logger.Printf("[DEBUG] Automatically reaping child processes")

pids := make(reap.PidCh, 1)
errors := make(reap.ErrorCh, 1)
go func() {
for {
select {
case pid := <-pids:
logger.Printf("[DEBUG] Reaped child process %d", pid)
case err := <-errors:
logger.Printf("[ERR] Error reaping child process: %v", err)
case <-c.agent.shutdownCh:
return
}
}
}()
go reap.ReapChildren(pids, errors, c.agent.shutdownCh, &c.agent.reapLock)
}
}

// Check and shut down the SCADA listeners at the end
defer func() {
if c.scadaHttp != nil {
Expand Down Expand Up @@ -829,7 +801,7 @@ func (c *Command) Run(args []string) int {
// Register the watches
for _, wp := range config.WatchPlans {
go func(wp *watch.WatchPlan) {
wp.Handler = makeWatchHandler(logOutput, wp.Exempt["handler"], &c.agent.reapLock)
wp.Handler = makeWatchHandler(logOutput, wp.Exempt["handler"])
wp.LogOutput = c.logOutput
if err := wp.Run(httpAddr.String()); err != nil {
c.Ui.Error(fmt.Sprintf("Error running watch: %v", err))
Expand Down Expand Up @@ -1017,7 +989,7 @@ func (c *Command) handleReload(config *Config) *Config {
// Register the new watches
for _, wp := range newConf.WatchPlans {
go func(wp *watch.WatchPlan) {
wp.Handler = makeWatchHandler(c.logOutput, wp.Exempt["handler"], &c.agent.reapLock)
wp.Handler = makeWatchHandler(c.logOutput, wp.Exempt["handler"])
wp.LogOutput = c.logOutput
if err := wp.Run(httpAddr.String()); err != nil {
c.Ui.Error(fmt.Sprintf("Error running watch: %v", err))
Expand Down
6 changes: 0 additions & 6 deletions command/agent/remote_exec.go
Original file line number Diff line number Diff line change
Expand Up @@ -135,12 +135,6 @@ func (a *Agent) handleRemoteExec(msg *UserEvent) {
return
}

// Disable child process reaping so that we can get this command's
// return value. Note that we take the read lock here since we are
// waiting on a specific PID and don't need to serialize all waits.
a.reapLock.RLock()
defer a.reapLock.RUnlock()

// Ensure we write out an exit code
exitCode := 0
defer a.remoteExecWriteExitCode(&event, &exitCode)
Expand Down
9 changes: 1 addition & 8 deletions command/agent/watch_handler.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ import (
"log"
"os"
"strconv"
"sync"

"github.com/armon/circbuf"
"github.com/hashicorp/consul/watch"
Expand All @@ -34,16 +33,10 @@ func verifyWatchHandler(params interface{}) error {
}

// makeWatchHandler returns a handler for the given watch
func makeWatchHandler(logOutput io.Writer, params interface{}, reapLock *sync.RWMutex) watch.HandlerFunc {
func makeWatchHandler(logOutput io.Writer, params interface{}) watch.HandlerFunc {
script := params.(string)
logger := log.New(logOutput, "", log.LstdFlags)
fn := func(idx uint64, data interface{}) {
// Disable child process reaping so that we can get this command's
// return value. Note that we take the read lock here since we are
// waiting on a specific PID and don't need to serialize all waits.
reapLock.RLock()
defer reapLock.RUnlock()

// Create the command
cmd, err := ExecScript(script)
if err != nil {
Expand Down
3 changes: 1 addition & 2 deletions command/agent/watch_handler_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ package agent
import (
"io/ioutil"
"os"
"sync"
"testing"
)

Expand All @@ -26,7 +25,7 @@ func TestMakeWatchHandler(t *testing.T) {
defer os.Remove("handler_out")
defer os.Remove("handler_index_out")
script := "echo $CONSUL_INDEX >> handler_index_out && cat >> handler_out"
handler := makeWatchHandler(os.Stderr, script, &sync.RWMutex{})
handler := makeWatchHandler(os.Stderr, script)
handler(100, []string{"foo", "bar", "baz"})
raw, err := ioutil.ReadFile("handler_out")
if err != nil {
Expand Down
Loading

0 comments on commit 24153f6

Please sign in to comment.