Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Open
benhoyt opened this issue May 22, 2022 · 5 comments · May be fixed by #124
Open

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

benhoyt opened this issue May 22, 2022 · 5 comments · May be fixed by #124

Comments

@benhoyt
Copy link
Owner

benhoyt commented May 22, 2022

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
Copy link
Owner Author

benhoyt commented May 24, 2022

Not a process group thing as such, but a pipe/stdout issue: golang/go#21922

@benhoyt benhoyt linked a pull request May 24, 2022 that will close this issue
Draft
benhoyt added a commit that referenced this issue May 24, 2022
@benhoyt
Skip TestExecuteContextSystemTimeout due to #122
6d17a08
@benhoyt benhoyt mentioned this issue May 24, 2022
Closed
@vegarsti
Copy link

vegarsti commented Aug 18, 2022 •
edited
Loading

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]

@benhoyt
Copy link
Owner Author

benhoyt commented Aug 18, 2022

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

@vegarsti
Copy link

vegarsti commented Aug 18, 2022 •
edited
Loading

@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

@benhoyt
Copy link
Owner Author

benhoyt commented Aug 19, 2022

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Make sure ctx being cancelled actually stops system() process benhoyt/goawk
2 participants
@benhoyt @vegarsti