diff --git a/cmd/proxy_test.go b/cmd/proxy_test.go index d609b62..a1e9a60 100644 --- a/cmd/proxy_test.go +++ b/cmd/proxy_test.go @@ -93,7 +93,7 @@ func Test_proxyUpstream(t *testing.T) { go func() { proxyUpstream(upstream, port, outputDir, tt.args.rewrite, false, tt.args.options) }() - if up := engine.WaitUntilUp(port, nil); !up { + if up, _ := engine.WaitUntilUp(port, nil); !up { t.Fatalf("proxy did not come up on port %d", port) } @@ -122,7 +122,7 @@ func Test_proxyUpstream(t *testing.T) { indexFileName = "GET-index.txt" } - if cfgExists := engine.WaitForOp(fmt.Sprintf("config file: %s", cfgFileName), 10*time.Second, nil, func() bool { + if cfgExists, _ := engine.WaitForOp(fmt.Sprintf("config file: %s", cfgFileName), 10*time.Second, nil, func() bool { if _, err = os.Stat(path.Join(outputDir, cfgFileName)); err != nil { return false } @@ -131,7 +131,7 @@ func Test_proxyUpstream(t *testing.T) { t.Fatalf("config file not found") } - if indexExists := engine.WaitForOp(fmt.Sprintf("index file: %s", indexFileName), 10*time.Second, nil, func() bool { + if indexExists, _ := engine.WaitForOp(fmt.Sprintf("index file: %s", indexFileName), 10*time.Second, nil, func() bool { if _, err = os.Stat(path.Join(outputDir, indexFileName)); err != nil { return false } @@ -155,7 +155,7 @@ func startUpstream() (server *http.Server, url string, port int, err error) { server.ListenAndServe() }() url = fmt.Sprintf("http://localhost:%d", port) - if up := engine.WaitForUrl("upstream", url, nil); !up { + if up, _ := engine.WaitForUrl("upstream", url, nil); !up { return nil, "", 0, fmt.Errorf("failed to start upstream on port %d", port) } return server, url, port, nil diff --git a/cmd/up.go b/cmd/up.go index fb9579c..23e5dea 100644 --- a/cmd/up.go +++ b/cmd/up.go @@ -30,6 +30,7 @@ import ( "path/filepath" "strings" "sync" + "sync/atomic" "syscall" ) @@ -216,27 +217,44 @@ func start(lib *engine.EngineLibrary, startOptions engine.StartOptions, configDi mockEngine := provider.Build(configDir, startOptions) wg := &sync.WaitGroup{} + var interrupted atomic.Bool if startOptions.IsDetached() { // DetachHealthy still traps Ctrl+C so an abort during the // healthcheck wait stops the mock; DetachNow returns immediately // so there is nothing to interrupt. if startOptions.Detach == engine.DetachHealthy { - trapExit(mockEngine, wg) + trapExit(mockEngine, wg, &interrupted) } if !mockEngine.Start(wg) { - // healthcheck timeout already calls logger.Fatalf; reaching - // here means the wait was aborted (e.g. Ctrl+C) + // on a healthcheck timeout the engine has already stopped the + // mock it started; exit non-zero so the failed start is + // visible. A false return with interrupted set means the wait + // was aborted (e.g. Ctrl+C), which is a clean shutdown. + if !interrupted.Load() { + os.Exit(1) + } return } printDetachSummary(mockEngine, startOptions) return } - trapExit(mockEngine, wg) + trapExit(mockEngine, wg, &interrupted) success := mockEngine.Start(wg) - if success && restartOnChange { + if !success { + // wait for any in-progress cleanup (the engine stops a mock that + // timed out; Ctrl+C triggers StopImmediately) to finish before + // returning, so we never leak a running mock. + wg.Wait() + if !interrupted.Load() { + os.Exit(1) + } + return + } + + if restartOnChange { dirUpdated := fileutil.WatchDir(configDir) go func() { for { @@ -259,12 +277,15 @@ func printDetachSummary(mockEngine engine.MockEngine, startOptions engine.StartO logger.Info("use 'imposter ls' to list running mocks, or 'imposter down' to stop them") } -// listen for an interrupt from the OS, then attempt engine cleanup -func trapExit(mockEngine engine.MockEngine, wg *sync.WaitGroup) { - c := make(chan os.Signal) +// listen for an interrupt from the OS, then attempt engine cleanup. +// interrupted is set before cleanup begins so the caller can tell an +// abort apart from a healthcheck timeout once Start returns. +func trapExit(mockEngine engine.MockEngine, wg *sync.WaitGroup, interrupted *atomic.Bool) { + c := make(chan os.Signal, 1) signal.Notify(c, os.Interrupt, syscall.SIGTERM) go func() { <-c + interrupted.Store(true) println() mockEngine.StopImmediately(wg) }() diff --git a/internal/engine/docker/engine.go b/internal/engine/docker/engine.go index 3f4b44c..a2275cb 100644 --- a/internal/engine/docker/engine.go +++ b/internal/engine/docker/engine.go @@ -107,20 +107,38 @@ func (d *DockerMockEngine) startWithOptions(wg *sync.WaitGroup, options engine.S case engine.DetachHealthy: // wait for health but don't stream logs or reap - the container // keeps running in dockerd after the CLI exits - return engine.WaitUntilUp(options.Port, d.shutDownC) + up, timedOut := engine.WaitUntilUp(options.Port, d.shutDownC) + if !up && timedOut { + d.stopAfterFailedStart(wg) + } + return up default: if err = streamLogsToStdIo(cli, ctx, containerId); err != nil { logger.Warn(err) } - up := engine.WaitUntilUp(options.Port, d.shutDownC) + up, timedOut := engine.WaitUntilUp(options.Port, d.shutDownC) + if !up { + if timedOut { + d.stopAfterFailedStart(wg) + } + return false + } // watch in case container stops go notifyOnStopBlocking(d, wg, containerId, cli, ctx) - return up + return true } } +// stopAfterFailedStart removes the container that was started but never +// became healthy, so a failed `up` does not leave an orphaned container +// running in dockerd. +func (d *DockerMockEngine) stopAfterFailedStart(wg *sync.WaitGroup) { + logger.Warnf("stopping mock that did not become healthy") + d.Stop(wg) +} + func (d *DockerMockEngine) GetID() string { if len(d.containerId) > 12 { return d.containerId[:12] diff --git a/internal/engine/health.go b/internal/engine/health.go index 1951305..0429ae4 100644 --- a/internal/engine/health.go +++ b/internal/engine/health.go @@ -67,7 +67,12 @@ func CheckMockStatus(port int) error { return fmt.Errorf("healthcheck status was %d for mock at %s: %s", resp.StatusCode, url, err) } -func WaitUntilUp(port int, shutDownC chan bool) (success bool) { +// WaitUntilUp blocks until the mock on the given port reports healthy. +// It returns success=true once healthy. If it returns success=false, +// timedOut distinguishes a healthcheck timeout (true) from an external +// abort via shutDownC, e.g. Ctrl+C (false), so callers can clean up a +// mock that never became healthy. +func WaitUntilUp(port int, shutDownC chan bool) (success bool, timedOut bool) { url := getStatusUrl(port) return WaitForUrl(fmt.Sprintf("status endpoint to return HTTP 200 at %v", url), url, shutDownC) } @@ -76,7 +81,7 @@ func getStatusUrl(port int) string { return fmt.Sprintf("http://localhost:%d/system/status", port) } -func WaitForUrl(desc string, url string, abortC chan bool) (success bool) { +func WaitForUrl(desc string, url string, abortC chan bool) (success bool, timedOut bool) { return WaitForOp(desc, getStartTimeout(), abortC, func() bool { resp, err := http.Get(url) if err != nil { @@ -90,7 +95,11 @@ func WaitForUrl(desc string, url string, abortC chan bool) (success bool) { }) } -func WaitForOp(desc string, timeoutDuration time.Duration, abortC chan bool, operation func() bool) (success bool) { +// WaitForOp polls operation until it succeeds, the timeout elapses, or an +// abort is received on abortC. On timeout it returns (false, true) rather +// than terminating the process, so the caller can stop any mock it started +// before exiting; an abort returns (false, false). +func WaitForOp(desc string, timeoutDuration time.Duration, abortC chan bool, operation func() bool) (success bool, timedOut bool) { logger.Tracef("waiting for %s", desc) successC := make(chan bool) @@ -107,21 +116,16 @@ func WaitForOp(desc string, timeoutDuration time.Duration, abortC chan bool, ope } }() - finished := false select { case <-timeout.C: - finished = true - logger.Fatalf("timed out waiting for %s", desc) - return false + logger.Errorf("timed out waiting for %s", desc) + return false, true case <-successC: - finished = true logger.Tracef("successfully waited for %s", desc) - return true + return true, false case <-abortC: - if !finished { - logger.Debugf("aborted waiting for %s", desc) - } - return false + logger.Debugf("aborted waiting for %s", desc) + return false, false } } diff --git a/internal/engine/health_test.go b/internal/engine/health_test.go new file mode 100644 index 0000000..62c4614 --- /dev/null +++ b/internal/engine/health_test.go @@ -0,0 +1,52 @@ +/* +Copyright © 2021 Pete Cornish + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package engine + +import ( + "testing" + "time" + + "github.com/stretchr/testify/assert" +) + +func Test_WaitForOp(t *testing.T) { + t.Run("succeeds when the operation passes", func(t *testing.T) { + success, timedOut := WaitForOp("op", time.Second, nil, func() bool { + return true + }) + assert.True(t, success) + assert.False(t, timedOut) + }) + + t.Run("reports timeout without terminating the process", func(t *testing.T) { + success, timedOut := WaitForOp("op", 50*time.Millisecond, nil, func() bool { + return false + }) + assert.False(t, success, "operation that never passes must not report success") + assert.True(t, timedOut, "timeout must be distinguishable so callers can clean up") + }) + + t.Run("reports abort separately from timeout", func(t *testing.T) { + abortC := make(chan bool, 1) + abortC <- true + success, timedOut := WaitForOp("op", time.Minute, abortC, func() bool { + return false + }) + assert.False(t, success) + assert.False(t, timedOut, "an external abort is not a timeout") + }) +} diff --git a/internal/engine/jvm/engine.go b/internal/engine/jvm/engine.go index 3bc96dd..2aad0fa 100644 --- a/internal/engine/jvm/engine.go +++ b/internal/engine/jvm/engine.go @@ -74,17 +74,34 @@ func (j *JvmMockEngine) startWithOptions(wg *sync.WaitGroup, options engine.Star return true case engine.DetachHealthy: // wait for health but do not reap - the OS reparents the child - return engine.WaitUntilUp(options.Port, j.shutDownC) + up, timedOut := engine.WaitUntilUp(options.Port, j.shutDownC) + if !up && timedOut { + j.stopAfterFailedStart(wg) + } + return up default: - up := engine.WaitUntilUp(options.Port, j.shutDownC) + up, timedOut := engine.WaitUntilUp(options.Port, j.shutDownC) + if !up { + if timedOut { + j.stopAfterFailedStart(wg) + } + return false + } // watch in case process stops go j.notifyOnStopBlocking(wg) - return up + return true } } +// stopAfterFailedStart kills the child process that was started but never +// became healthy, so a failed `up` does not leave an orphaned mock behind. +func (j *JvmMockEngine) stopAfterFailedStart(wg *sync.WaitGroup) { + logger.Warnf("stopping mock that did not become healthy") + j.Stop(wg) +} + func (j *JvmMockEngine) GetID() string { if j.command == nil || j.command.Process == nil { return "" diff --git a/internal/engine/native/engine.go b/internal/engine/native/engine.go index 17c44e7..95cddd6 100644 --- a/internal/engine/native/engine.go +++ b/internal/engine/native/engine.go @@ -74,16 +74,33 @@ func (g *NativeMockEngine) startWithOptions(wg *sync.WaitGroup, options engine.S return true case engine.DetachHealthy: // wait for health but do not reap - the OS reparents the child - return engine.WaitUntilUp(options.Port, g.shutDownC) + up, timedOut := engine.WaitUntilUp(options.Port, g.shutDownC) + if !up && timedOut { + g.stopAfterFailedStart(wg) + } + return up default: - // watch in case process stops - up := engine.WaitUntilUp(options.Port, g.shutDownC) + up, timedOut := engine.WaitUntilUp(options.Port, g.shutDownC) + if !up { + if timedOut { + g.stopAfterFailedStart(wg) + } + return false + } + // watch in case process stops go g.notifyOnStopBlocking(wg) - return up + return true } } +// stopAfterFailedStart kills the child process that was started but never +// became healthy, so a failed `up` does not leave an orphaned mock behind. +func (g *NativeMockEngine) stopAfterFailedStart(wg *sync.WaitGroup) { + logger.Warnf("stopping mock that did not become healthy") + g.Stop(wg) +} + func (g *NativeMockEngine) GetID() string { if g.cmd == nil || g.cmd.Process == nil { return ""