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.

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

x/playground: send SIGQUIT on timeout to provide useful context #38343

Open
toothrot opened this issue Apr 10, 2020 · 2 comments
Open

x/playground: send SIGQUIT on timeout to provide useful context #38343

toothrot opened this issue Apr 10, 2020 · 2 comments
Labels
NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone

Comments

@toothrot
Copy link
Contributor

The playground could kill processes in a way that provided the user helpful context instead of Process.Kill().

What version of Go are you using (go version)?

$ go version
go version go1.14 linux/amd64 (playground)

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

go env Output
$ go env

What did you do?

package main

func main() {
	for {
	}
}

What did you expect to see?

timeout running program
SIGQUIT: quit
PC=0x458050 m=0 sigcode=0

goroutine 1 [running]:
main.main()
	/tmp/sandbox300849141/prog.go:4 fp=0xc000034788 sp=0xc000034780 pc=0x458050
runtime.main()
	/usr/local/go-faketime/src/runtime/proc.go:203 +0x20e fp=0xc0000347e0 sp=0xc000034788 pc=0x42bf4e
runtime.goexit()
	/usr/local/go-faketime/src/runtime/asm_amd64.s:1373 +0x1 fp=0xc0000347e8 sp=0xc0000347e0 pc=0x453831

rax    0x458050
rbx    0x0
rcx    0x4d0000
rdx    0x47a788
rdi    0xc000060010
rsi    0x0
rbp    0xc0000347d0
rsp    0xc000034780
r8     0x0
r9     0x1
r10    0xc000060000
r11    0x1
r12    0x0
r13    0x40
r14    0x483ecc
r15    0x0
rip    0x458050
rflags 0x246
cs     0x33
fs     0x0
gs     0x0

Program exited: status 2.

What did you see instead?

timeout running program

/cc @bcmills @cagedmantis @dmitshur

@toothrot toothrot added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Apr 10, 2020
@toothrot toothrot added this to the Unreleased milestone Apr 10, 2020
@toothrot toothrot self-assigned this Apr 10, 2020
@gopherbot
Copy link

Change https://golang.org/cl/227652 mentions this issue: sandbox: timeout runsc commands

@toothrot
Copy link
Contributor Author

We should also clean up usages of CommandContext when we want to send a friendlier signal (golang.org/cl/227351 comment)

gopherbot pushed a commit to golang/playground that referenced this issue Apr 15, 2020
The current mechanism for forcing a process to die ater a timeout is not
sufficient. This change fixes issues that were causing processes to run
forever on the sandbox.

- Gracefully terminate processes before we kill them inside of our
gVisor process. This helps capture valuable debug output for the user.

- Return a friendlier error when our run context times out on the
playground.

- Add a test that timeouts are handled gracefully.

- Reduce concurrent goroutines in our sandbox run handler by replacing
goroutine copy functions with a custom writer (limitedWriter) that
returns an error if too much output is returned, halting the program.

- Custom writers (limitedWriter, switchWriter) also fix timing errors
when calling Wait() too soon on a Command, before we have read all of
the data. It also fixes a different error from trying to read data after
a program has terminated.

- Remove goroutine from startContainer, and use a ticker + context
timeout for synchronization.

Updates golang/go#25224
Updates golang/go#38343

Change-Id: Ie9d65220e5c4f39272ea70b45c4b472bcd7069bb
Reviewed-on: https://go-review.googlesource.com/c/playground/+/227652
Run-TryBot: Alexander Rakoczy <alex@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Bryan C. Mills <bcmills@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
None yet
Development

No branches or pull requests

2 participants