Make sure ctx being cancelled actually stops system() process

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
 }
 

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 {

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 {