Use WaitDelay to ensure system() exits quickly when context is cancelled (#270)

I don't think this is an exhaustive fix, but it fixes the main issue of
system() never returning when the context is cancelled (due to
stdout/stderr pipes being passed to the child process).

This PR also bumps up the minimum Go version required in go.mod from
1.18 to 1.20, as `Cmd.WaitDelay` was only introduced in Go 1.20. That's
fairly old by now, so this seems reasonable.

Fixes #122

Author: benhoyt

.github/workflows/tests.yml

@@ -13,7 +13,7 @@ jobs:
     strategy:
       fail-fast: false
       matrix:
-        go: ['1.25', '1.24', '1.23', '1.18']
+        go: ['1.25', '1.24', '1.23', '1.20']
 
     name: Go ${{ matrix.go }} on Linux
 
@@ -59,7 +59,7 @@ jobs:
     strategy:
       fail-fast: false
       matrix:
-        go: ['1.25', '1.18']
+        go: ['1.25', '1.20']
 
     name: Go ${{ matrix.go }} on Windows
 
@@ -85,7 +85,7 @@ jobs:
     strategy:
       fail-fast: false
       matrix:
-        go: ['1.25']  # TODO: '1.18' on macOS not working right now (403 when downloading)
+        go: ['1.25', '1.20']
 
     name: Go ${{ matrix.go }} on macOS
 

go.mod

@@ -1,3 +1,3 @@
 module github.com/benhoyt/goawk
 
-go 1.18
+go 1.20

interp/io.go

@@ -13,6 +13,7 @@ import (
 	"regexp"
 	"strconv"
 	"strings"
+	"time"
 	"unicode/utf8"
 
 	"github.com/benhoyt/goawk/internal/resolver"
@@ -170,11 +171,16 @@ func (p *interp) execShell(code string) *exec.Cmd {
 	executable := p.shellCommand[0]
 	args := p.shellCommand[1:]
 	args = append(args, code)
+
+	var cmd *exec.Cmd
 	if p.checkCtx {
-		return exec.CommandContext(p.ctx, executable, args...)
+		cmd = exec.CommandContext(p.ctx, executable, args...)
 	} else {
-		return exec.Command(executable, args...)
+		cmd = exec.Command(executable, args...)
 	}
+	// Ensure stdout/stderr pipes being held open don't keep process running.
+	cmd.WaitDelay = 250 * time.Millisecond
+	return cmd
 }
 
 // Get input Scanner to use for "getline" based on file name

interp/newexecute.go

@@ -163,8 +163,8 @@ func (p *Interpreter) ResetRand() {
 // set an execution timeout or cancel the execution. For efficiency, the
 // context is only tested every 1000 virtual machine instructions.
 //
-// Context handling is not preemptive: currently long-running operations like
-// system() won't be interrupted.
+// Context handling is not preemptive: some long-running operations may not
+// be interrupted when the context is cancelled.
 func (p *Interpreter) ExecuteContext(ctx context.Context, config *Config) (int, error) {
 	p.interp.resetCore()
 	p.interp.checkCtx = ctx != context.Background() && ctx != context.TODO()

interp/newexecute_test.go

@@ -7,6 +7,7 @@ import (
 	"context"
 	"errors"
 	"math"
+	"runtime"
 	"strings"
 	"testing"
 	"time"
@@ -167,13 +168,25 @@ func TestExecuteContextCancel(t *testing.T) {
 }
 
 func TestExecuteContextSystemTimeout(t *testing.T) {
-	t.Skip("TODO: skipping for now due to #122")
-	interpreter := newInterp(t, `BEGIN { print system("sleep 4") }`)
+	start := 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)
-	if !errors.Is(err, context.DeadlineExceeded) {
-		t.Fatalf("expected DeadlineExceeded error, got: %v", err)
+	switch runtime.GOOS {
+	case "windows", "darwin":
+		// For some reason on Windows and macOS the returned error is nil, rather
+		// than DeadlineExceeded. Don't check it on those platforms.
+	default:
+		if !errors.Is(err, context.DeadlineExceeded) {
+			t.Fatalf("expected DeadlineExceeded error, got: %v", err)
+		}
+	}
+
+	elapsed := time.Since(start)
+	if elapsed > 500*time.Millisecond {
+		t.Fatalf("should have taken ~5ms, took %v", elapsed)
 	}
 }