goawk icon indicating copy to clipboard operation
goawk copied to clipboard

Cancelling ctx in system() doesn't actually stop it

Open benhoyt opened this issue 3 years ago • 5 comments

TestExecuteContextSystemTimeout takes 4s, indicating that the cancellation of the context isn't actually killing the "sleep 4" command. Presumably I was wrong about being wrong here. :-) I guess we need to kill the process group after all.

benhoyt avatar May 22 '22 08:05 benhoyt

Not a process group thing as such, but a pipe/stdout issue: https://github.com/golang/go/issues/21922

benhoyt avatar May 24 '22 04:05 benhoyt

This test takes 0.8s on macOS for me!

$ go test ./... -run TestExecuteContextSystemTimeout
ok      github.com/benhoyt/goawk        0.410s [no tests to run]
ok      github.com/benhoyt/goawk/internal/ast   0.757s [no tests to run]
ok      github.com/benhoyt/goawk/internal/compiler      0.885s [no tests to run]
ok      github.com/benhoyt/goawk/interp 0.842s
ok      github.com/benhoyt/goawk/lexer  0.483s [no tests to run]
ok      github.com/benhoyt/goawk/parser 0.616s [no tests to run]
?       github.com/benhoyt/goawk/scripts/csvbench/count [no test files]
?       github.com/benhoyt/goawk/scripts/csvbench/write [no test files]

vegarsti avatar Aug 18 '22 21:08 vegarsti

@vegarsti Was that on the branch, or master? Because on master I'm actually skipping that specific test for now due to this issue: https://github.com/benhoyt/goawk/commit/6d17a0874237496991daf86ea3098cd3d634e8a5 (I'm not sure where the 0.8s is coming from if you're on master)

benhoyt avatar Aug 18 '22 21:08 benhoyt

@benhoyt This is master! But you're absolutely right, that run did skip the test. But if I remove the t.Skip(), I get 0.01s.

go test -v ./interp -run "^TestExecuteContextSystemTimeout$"
=== RUN   TestExecuteContextSystemTimeout
--- PASS: TestExecuteContextSystemTimeout (0.01s)
PASS
ok      github.com/benhoyt/goawk/interp 0.114s

vegarsti avatar Aug 18 '22 21:08 vegarsti

Very weird! I get 4s consistently:

$ go test ./interp -run=TestExecuteContextSystemTimeout -v -count=1
=== RUN   TestExecuteContextSystemTimeout
--- PASS: TestExecuteContextSystemTimeout (4.00s)
PASS
ok  	github.com/benhoyt/goawk/interp	4.005s

benhoyt avatar Aug 19 '22 01:08 benhoyt