From 4ad97bbb7cf065d34f2e6134e86221c62dbc0172 Mon Sep 17 00:00:00 2001 From: Duncan Holm Date: Fri, 23 Jun 2017 21:02:39 +0100 Subject: [PATCH 1/8] First take of 'pipe signals' feature for Windows --- cmd/modd/main.go | 14 +++++++++++++- conf/block_windows.go | 9 +++++++-- conf/conf.go | 5 +++-- daemon.go | 18 ++++++++++++++++-- modd.go | 11 ++++++++++- 5 files changed, 49 insertions(+), 8 deletions(-) diff --git a/cmd/modd/main.go b/cmd/modd/main.go index 1738275..34a9a41 100644 --- a/cmd/modd/main.go +++ b/cmd/modd/main.go @@ -3,6 +3,7 @@ package main import ( "fmt" "os" + "runtime" "github.com/cortesi/modd" "github.com/cortesi/modd/notify" @@ -46,9 +47,20 @@ var debug = kingpin.Flag("debug", "Debugging for modd development"). Default("false"). Bool() +var pipesignals = new(bool) + func main() { kingpin.CommandLine.HelpFlag.Short('h') kingpin.Version(modd.Version) + + if runtime.GOOS == "windows" { + pipesignals = kingpin.Flag( + "pipesignals", + "For signals that don't exist on Windows, write their name to the stdin of daemons instead"). + Default("true"). + Bool() + } + kingpin.Parse() if *ignores { @@ -77,7 +89,7 @@ func main() { notifiers = append(notifiers, ¬ify.BeepNotifier{}) } - mr, err := modd.NewModRunner(*file, log, notifiers, !(*noconf)) + mr, err := modd.NewModRunner(*file, log, notifiers, !(*noconf), *pipesignals) if err != nil { log.Shout("%s", err) return diff --git a/conf/block_windows.go b/conf/block_windows.go index 08fb8eb..ef0b33a 100644 --- a/conf/block_windows.go +++ b/conf/block_windows.go @@ -12,8 +12,9 @@ func (b *Block) addDaemon(command string, options []string) error { b.Daemons = []Daemon{} } d := Daemon{ - Command: command, - RestartSignal: syscall.SIGHUP, + Command: command, + RestartSignal: syscall.SIGHUP, + PipeRestartSignal: true, } for _, v := range options { switch v { @@ -25,6 +26,10 @@ func (b *Block) addDaemon(command string, options []string) error { d.RestartSignal = syscall.SIGINT case "+sigkill": d.RestartSignal = syscall.SIGKILL + // Although Windows doesn't have signals, Go does recognise the + // intention of SIGKILL and uses a native API to terminate the + // target process. + d.PipeRestartSignal = false case "+sigquit": d.RestartSignal = syscall.SIGQUIT default: diff --git a/conf/conf.go b/conf/conf.go index 7b6eb94..894fd48 100644 --- a/conf/conf.go +++ b/conf/conf.go @@ -11,8 +11,9 @@ import ( // A Daemon is a persistent process that is kept running type Daemon struct { - Command string - RestartSignal os.Signal + Command string + RestartSignal os.Signal + PipeRestartSignal bool } // A Prep runs and terminates diff --git a/daemon.go b/daemon.go index 3a5201f..17a50d0 100644 --- a/daemon.go +++ b/daemon.go @@ -1,6 +1,7 @@ package modd import ( + "io" "os" "os/exec" "sync" @@ -28,6 +29,7 @@ type daemon struct { log termlog.Stream cmd *exec.Cmd + stdin io.Writer shell string stop bool started bool @@ -51,6 +53,13 @@ func (d *daemon) Run() { return } c.Dir = d.indir + if d.conf.PipeRestartSignal { + d.stdin, err = c.StdinPipe() + if err != nil { + d.log.Shout("%s", err) + continue + } + } stdo, err := c.StdoutPipe() if err != nil { d.log.Shout("%s", err) @@ -106,8 +115,13 @@ func (d *daemon) Restart() { d.started = true } else { if d.cmd != nil { - d.log.Notice(">> sending signal %s", d.conf.RestartSignal) - d.cmd.Process.Signal(d.conf.RestartSignal) + if d.conf.PipeRestartSignal { + d.log.Notice(">> piping signal %s", d.conf.RestartSignal) + d.stdin.Write([]byte(d.conf.RestartSignal.String() + "\n")) + } else { + d.log.Notice(">> sending signal %s", d.conf.RestartSignal) + d.cmd.Process.Signal(d.conf.RestartSignal) + } } } } diff --git a/modd.go b/modd.go index 2ac82ca..d107304 100644 --- a/modd.go +++ b/modd.go @@ -61,7 +61,7 @@ type ModRunner struct { } // NewModRunner constructs a new ModRunner -func NewModRunner(confPath string, log termlog.TermLog, notifiers []notify.Notifier, confreload bool) (*ModRunner, error) { +func NewModRunner(confPath string, log termlog.TermLog, notifiers []notify.Notifier, confreload, pipesignals bool) (*ModRunner, error) { mr := &ModRunner{ Log: log, ConfPath: confPath, @@ -72,6 +72,15 @@ func NewModRunner(confPath string, log termlog.TermLog, notifiers []notify.Notif if err != nil { return nil, err } + // On Windows, daemon config structs are created with the assumption that + // the pipesignals feature will be used in cases where the alternative is + // the signal silently failing. If the feature has been explicitly disabled + // using the command-line flag, correct that assumption. + for _, blockcnf := range mr.Config.Blocks { + for _, daemoncnf := range blockcnf.Daemons { + daemoncnf.PipeRestartSignal = daemoncnf.PipeRestartSignal && pipesignals + } + } return mr, nil } From 767e3358d6f1689f40cb011162a925c201b3ed26 Mon Sep 17 00:00:00 2001 From: Duncan Holm Date: Thu, 29 Jun 2017 22:01:24 +0100 Subject: [PATCH 2/8] Fix modified PipeRestartSignal flag not persisting The conf structs are looped over by-value, so the flag was being set on a copy of the daemon conf struct. --- modd.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/modd.go b/modd.go index d107304..68dc9b3 100644 --- a/modd.go +++ b/modd.go @@ -76,9 +76,10 @@ func NewModRunner(confPath string, log termlog.TermLog, notifiers []notify.Notif // the pipesignals feature will be used in cases where the alternative is // the signal silently failing. If the feature has been explicitly disabled // using the command-line flag, correct that assumption. - for _, blockcnf := range mr.Config.Blocks { - for _, daemoncnf := range blockcnf.Daemons { - daemoncnf.PipeRestartSignal = daemoncnf.PipeRestartSignal && pipesignals + for ib, blockcnf := range mr.Config.Blocks { + for id, daemoncnf := range blockcnf.Daemons { + defaultVal := daemoncnf.PipeRestartSignal + mr.Config.Blocks[ib].Daemons[id].PipeRestartSignal = defaultVal && pipesignals } } return mr, nil From 88f755db27d00be6161f7b20d024583a0a5f61b8 Mon Sep 17 00:00:00 2001 From: Duncan Holm Date: Tue, 14 Nov 2017 13:16:16 +0000 Subject: [PATCH 3/8] Correct non-present-tense Windows signal strings: cf. `var signals` in `ztypes_windows.go` in Go's standard library. --- daemon.go | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/daemon.go b/daemon.go index 17a50d0..7d4b035 100644 --- a/daemon.go +++ b/daemon.go @@ -1,6 +1,7 @@ package modd import ( + "fmt" "io" "os" "os/exec" @@ -106,6 +107,16 @@ func (d *daemon) Run() { } } +// Go's standard library uses a mix of word tenses for the string +// representation of signals. For our 'piped signals' feature, the strings we +// write on the pipe should be present-tense because they are conceptually +// requests. +var pipedSignalTenseCorrections = map[string]string{ + "aborted": "abort", + "killed": "kill", + "terminated": "terminate", +} + // Restart the daemon, or start it if it's not yet running func (d *daemon) Restart() { d.Lock() @@ -117,7 +128,11 @@ func (d *daemon) Restart() { if d.cmd != nil { if d.conf.PipeRestartSignal { d.log.Notice(">> piping signal %s", d.conf.RestartSignal) - d.stdin.Write([]byte(d.conf.RestartSignal.String() + "\n")) + sigStr := d.conf.RestartSignal.String() + if s, ok := pipedSignalTenseCorrections[sigStr]; ok { + sigStr = s + } + fmt.Fprintln(d.stdin, sigStr) } else { d.log.Notice(">> sending signal %s", d.conf.RestartSignal) d.cmd.Process.Signal(d.conf.RestartSignal) From 3ba4b2bd227460e72e33942e0e85f6f5a0b93235 Mon Sep 17 00:00:00 2001 From: Duncan Holm Date: Tue, 14 Nov 2017 15:56:05 +0000 Subject: [PATCH 4/8] s/piped/pipe ; Refine override loop --- daemon.go | 6 +++--- modd.go | 18 ++++++++++-------- 2 files changed, 13 insertions(+), 11 deletions(-) diff --git a/daemon.go b/daemon.go index 7d4b035..e119f6d 100644 --- a/daemon.go +++ b/daemon.go @@ -108,10 +108,10 @@ func (d *daemon) Run() { } // Go's standard library uses a mix of word tenses for the string -// representation of signals. For our 'piped signals' feature, the strings we +// representation of signals. For our 'pipe signals' feature, the strings we // write on the pipe should be present-tense because they are conceptually // requests. -var pipedSignalTenseCorrections = map[string]string{ +var pipeSignalTenseCorrections = map[string]string{ "aborted": "abort", "killed": "kill", "terminated": "terminate", @@ -129,7 +129,7 @@ func (d *daemon) Restart() { if d.conf.PipeRestartSignal { d.log.Notice(">> piping signal %s", d.conf.RestartSignal) sigStr := d.conf.RestartSignal.String() - if s, ok := pipedSignalTenseCorrections[sigStr]; ok { + if s, ok := pipeSignalTenseCorrections[sigStr]; ok { sigStr = s } fmt.Fprintln(d.stdin, sigStr) diff --git a/modd.go b/modd.go index 68dc9b3..e16e283 100644 --- a/modd.go +++ b/modd.go @@ -72,14 +72,16 @@ func NewModRunner(confPath string, log termlog.TermLog, notifiers []notify.Notif if err != nil { return nil, err } - // On Windows, daemon config structs are created with the assumption that - // the pipesignals feature will be used in cases where the alternative is - // the signal silently failing. If the feature has been explicitly disabled - // using the command-line flag, correct that assumption. - for ib, blockcnf := range mr.Config.Blocks { - for id, daemoncnf := range blockcnf.Daemons { - defaultVal := daemoncnf.PipeRestartSignal - mr.Config.Blocks[ib].Daemons[id].PipeRestartSignal = defaultVal && pipesignals + // When the daemon config structs were constructed, their + // .PipeRestartSignal boolean fields were enabled in cases where their + // chosen restart signals would otherwise fail silently. If the 'pipe + // signals' feature has been explicitly disabled by command-line flag then + // those smart defaults need to be cleared. + if !pipesignals { + for i := 0; i < len(mr.Config.Blocks); i++ { + for j := 0; j < len(mr.Config.Blocks[i].Daemons); j++ { + mr.Config.Blocks[i].Daemons[j].PipeRestartSignal = false + } } } return mr, nil From ddcb7f981e24ee1771a564d82fb5179bb34d99bc Mon Sep 17 00:00:00 2001 From: Duncan Holm Date: Tue, 14 Nov 2017 15:56:30 +0000 Subject: [PATCH 5/8] Fix non-corrected signal string being logged --- daemon.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/daemon.go b/daemon.go index e119f6d..47e1819 100644 --- a/daemon.go +++ b/daemon.go @@ -127,11 +127,11 @@ func (d *daemon) Restart() { } else { if d.cmd != nil { if d.conf.PipeRestartSignal { - d.log.Notice(">> piping signal %s", d.conf.RestartSignal) sigStr := d.conf.RestartSignal.String() if s, ok := pipeSignalTenseCorrections[sigStr]; ok { sigStr = s } + d.log.Notice(">> piping signal \"%s\"", sigStr) fmt.Fprintln(d.stdin, sigStr) } else { d.log.Notice(">> sending signal %s", d.conf.RestartSignal) From a66709a6477fb779f718ff3d1a0b3938accdba80 Mon Sep 17 00:00:00 2001 From: Duncan Holm Date: Mon, 27 Nov 2017 10:49:50 +0000 Subject: [PATCH 6/8] Extract method; Add TODO --- cmd/modd/main.go | 3 ++- modd.go | 25 +++++++++++++++---------- 2 files changed, 17 insertions(+), 11 deletions(-) diff --git a/cmd/modd/main.go b/cmd/modd/main.go index 34a9a41..e173f73 100644 --- a/cmd/modd/main.go +++ b/cmd/modd/main.go @@ -57,7 +57,8 @@ func main() { pipesignals = kingpin.Flag( "pipesignals", "For signals that don't exist on Windows, write their name to the stdin of daemons instead"). - Default("true"). + // TODO(DH): Should it be enabled by default? + Default("false"). Bool() } diff --git a/modd.go b/modd.go index e16e283..3b765ff 100644 --- a/modd.go +++ b/modd.go @@ -72,17 +72,8 @@ func NewModRunner(confPath string, log termlog.TermLog, notifiers []notify.Notif if err != nil { return nil, err } - // When the daemon config structs were constructed, their - // .PipeRestartSignal boolean fields were enabled in cases where their - // chosen restart signals would otherwise fail silently. If the 'pipe - // signals' feature has been explicitly disabled by command-line flag then - // those smart defaults need to be cleared. if !pipesignals { - for i := 0; i < len(mr.Config.Blocks); i++ { - for j := 0; j < len(mr.Config.Blocks[i].Daemons); j++ { - mr.Config.Blocks[i].Daemons[j].PipeRestartSignal = false - } - } + mr.disablePipeSignals() } return mr, nil } @@ -108,6 +99,20 @@ func (mr *ModRunner) ReadConfig() error { return nil } +// disablePipeSignals disables the PipeRestartSignal flag in all daemon config +// structs. When those structs were constructed, that flag was enabled in cases +// where the combination of the running host OS and the specific signal chosen +// would result in that signal silently failing to do anything. If the entire +// 'pipe signals' feature is to be disabled, then those smart defaults need to +// be cleared by calling this method. +func (mr *ModRunner) disablePipeSignals() { + for i := 0; i < len(mr.Config.Blocks); i++ { + for j := 0; j < len(mr.Config.Blocks[i].Daemons); j++ { + mr.Config.Blocks[i].Daemons[j].PipeRestartSignal = false + } + } +} + // PrepOnly runs all prep functions and exits func (mr *ModRunner) PrepOnly(initial bool) error { for _, b := range mr.Config.Blocks { From 1c7acd20b97ba1ec2f558a3adfe091da69a9559d Mon Sep 17 00:00:00 2001 From: Duncan Holm Date: Mon, 27 Nov 2017 20:56:37 +0000 Subject: [PATCH 7/8] Fix tests lacking new struct field --- conf/parse_test.go | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/conf/parse_test.go b/conf/parse_test.go index 1569df4..f2477c3 100644 --- a/conf/parse_test.go +++ b/conf/parse_test.go @@ -1,6 +1,7 @@ package conf import ( + "runtime" "syscall" "testing" ) @@ -96,7 +97,7 @@ var parseTests = []struct { Blocks: []Block{ { Include: []string{"foo"}, - Daemons: []Daemon{{"command", syscall.SIGHUP}}, + Daemons: []Daemon{{"command", syscall.SIGHUP, windows}}, }, }, }, @@ -105,25 +106,25 @@ var parseTests = []struct { "{\ndaemon +sighup: c\n}", &Config{ Blocks: []Block{ - {Daemons: []Daemon{{"c", syscall.SIGHUP}}}, + {Daemons: []Daemon{{"c", syscall.SIGHUP, windows}}}, }, }, }, { "{\ndaemon +sigterm: c\n}", - &Config{Blocks: []Block{{Daemons: []Daemon{{"c", syscall.SIGTERM}}}}}, + &Config{Blocks: []Block{{Daemons: []Daemon{{"c", syscall.SIGTERM, windows}}}}}, }, { "{\ndaemon +sigint: c\n}", - &Config{Blocks: []Block{{Daemons: []Daemon{{"c", syscall.SIGINT}}}}}, + &Config{Blocks: []Block{{Daemons: []Daemon{{"c", syscall.SIGINT, windows}}}}}, }, { "{\ndaemon +sigkill: c\n}", - &Config{Blocks: []Block{{Daemons: []Daemon{{"c", syscall.SIGKILL}}}}}, + &Config{Blocks: []Block{{Daemons: []Daemon{{"c", syscall.SIGKILL, false}}}}}, }, { "{\ndaemon +sigquit: c\n}", - &Config{Blocks: []Block{{Daemons: []Daemon{{"c", syscall.SIGQUIT}}}}}, + &Config{Blocks: []Block{{Daemons: []Daemon{{"c", syscall.SIGQUIT, windows}}}}}, }, { "foo {\nprep: command\n}", @@ -243,6 +244,8 @@ var parseTests = []struct { }, } +var windows = runtime.GOOS == "windows" + func TestParse(t *testing.T) { for i, tt := range parseTests { ret, err := Parse("test", tt.input) From e87899cfe8f146357d4e5495d9eab24a435c5515 Mon Sep 17 00:00:00 2001 From: Duncan Holm Date: Tue, 28 Nov 2017 09:09:01 +0000 Subject: [PATCH 8/8] Fill out struct initializer in posix tests too --- conf/parse_posix_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/conf/parse_posix_test.go b/conf/parse_posix_test.go index 4b8bf62..52051e0 100644 --- a/conf/parse_posix_test.go +++ b/conf/parse_posix_test.go @@ -12,15 +12,15 @@ var parsePosixTests = []struct { }{ { "{\ndaemon +sigusr1: c\n}", - &Config{Blocks: []Block{{Daemons: []Daemon{{"c", syscall.SIGUSR1}}}}}, + &Config{Blocks: []Block{{Daemons: []Daemon{{"c", syscall.SIGUSR1, false}}}}}, }, { "{\ndaemon +sigusr2: c\n}", - &Config{Blocks: []Block{{Daemons: []Daemon{{"c", syscall.SIGUSR2}}}}}, + &Config{Blocks: []Block{{Daemons: []Daemon{{"c", syscall.SIGUSR2, false}}}}}, }, { "{\ndaemon +sigwinch: c\n}", - &Config{Blocks: []Block{{Daemons: []Daemon{{"c", syscall.SIGWINCH}}}}}, + &Config{Blocks: []Block{{Daemons: []Daemon{{"c", syscall.SIGWINCH, false}}}}}, }, }