diff --git a/interp/interp_test.go b/interp/interp_test.go index d1a4ba0f..e62ce35a 100644 --- a/interp/interp_test.go +++ b/interp/interp_test.go @@ -906,6 +906,7 @@ func testGoAWK( t *testing.T, src, in, out, errStr string, funcs map[string]interface{}, configure func(config *interp.Config), ) { + t.Helper() parserConfig := &parser.ParserConfig{ Funcs: funcs, } @@ -1302,7 +1303,7 @@ func TestSystemCommandNotFound(t *testing.T) { } type mockFlusher struct { - bytes.Buffer + concurrentBuffer flushes []string } diff --git a/interp/newexecute_test.go b/interp/newexecute_test.go index 7c74e592..6d814874 100644 --- a/interp/newexecute_test.go +++ b/interp/newexecute_test.go @@ -6,6 +6,8 @@ import ( "bytes" "context" "errors" + "io/ioutil" + "os" "strings" "testing" "time" @@ -138,14 +140,38 @@ func TestExecuteContextCancel(t *testing.T) { } } -func TestExecuteContextSystemTimeout(t *testing.T) { - interpreter := newInterp(t, `BEGIN { print system("sleep 4") }`) +// The three cases here are to ensure we test https://github.com/benhoyt/goawk/issues/122 +func TestExecuteContextSystemTimeoutDefault(t *testing.T) { + testExecuteContextSystemTimeout(t, nil) +} + +func TestExecuteContextSystemTimeoutStdoutStderr(t *testing.T) { + testExecuteContextSystemTimeout(t, &interp.Config{ + Output: os.Stdout, + Error: os.Stderr, + }) +} + +func TestExecuteContextSystemTimeoutDiscard(t *testing.T) { + testExecuteContextSystemTimeout(t, &interp.Config{ + Output: ioutil.Discard, + Error: ioutil.Discard, + }) +} + +func testExecuteContextSystemTimeout(t *testing.T, config *interp.Config) { + started := time.Now() + interpreter := newInterp(t, `BEGIN { print system("sleep 1") }`) ctx, cancel := context.WithTimeout(context.Background(), 5*time.Millisecond) defer cancel() - _, err := interpreter.ExecuteContext(ctx, nil) + _, err := interpreter.ExecuteContext(ctx, config) if !errors.Is(err, context.DeadlineExceeded) { t.Fatalf("expected DeadlineExceeded error, got: %v", err) } + elapsed := time.Since(started) + if elapsed > 500*time.Millisecond { + t.Fatalf("should have taken ~5ms, took %v", elapsed) + } } func newInterp(t *testing.T, src string) *interp.Interpreter { diff --git a/interp/vm.go b/interp/vm.go index 49b07f50..204cf395 100644 --- a/interp/vm.go +++ b/interp/vm.go @@ -1068,10 +1068,45 @@ func (p *interp) callBuiltin(builtinOp compiler.BuiltinOp) error { cmdline := p.toString(p.peekTop()) cmd := p.execShell(cmdline) cmd.Stdin = p.stdin - cmd.Stdout = p.output - cmd.Stderr = p.errorOutput + _ = p.flushAll() // ensure synchronization - err := cmd.Run() + + // To ensure we address https://github.com/golang/go/issues/21922, use + // pipes instead of cmd.Stdout and cmd.Stderr directly, otherwise when + // the context is cancelled the (grand)child process won't terminate. + stdoutPipe, err := cmd.StdoutPipe() + if err != nil { + return err + } + stderrPipe, err := cmd.StderrPipe() + if err != nil { + return err + } + + // Start the process (before reading from stdout/stderr). + err = cmd.Start() + if p.checkCtx && err == nil && p.ctx.Err() != nil { + err = p.ctx.Err() + } + if err == nil { + stdoutCh := make(chan error) + go func() { + io.Copy(p.output, stdoutPipe) + close(stdoutCh) + }() + stderrCh := make(chan error) + go func() { + io.Copy(p.errorOutput, stderrPipe) + close(stderrCh) + }() + + // Wait till process completes. + err = cmd.Wait() + + // Ensure copy goroutines have exited. + <-stdoutCh + <-stderrCh + } ret := 0.0 if err != nil { if p.checkCtx && p.ctx.Err() != nil {