Skip to content

Commit 0084f7f

Browse files
toothrottoothrot
committed
playground: show a meaningful error on build timeout
Add support for returning a meaningful error message to the user, along with partial build output if present. This change also improves graceful command termination throughout the playground. It introduces internal.WaitOrStop, which will wait for a command to finish, or gracefully stop the command if it has not finished by the time the context is cancelled. This also resolves comments from CL 227351. Updates golang/go#38052 Updates golang/go#38530 Updates golang/go#25224 Change-Id: I3a36ad2c5f3626c9cd5b3c1139543ccde3e17ba1 Reviewed-on: https://go-review.googlesource.com/c/playground/+/228438 Run-TryBot: Alexander Rakoczy <alex@golang.org> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Bryan C. Mills <bcmills@google.com> Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
1 parent ede79dd commit 0084f7f

File tree

3 files changed

+153
-84
lines changed

3 files changed

+153
-84
lines changed

‎internal/internal.go‎

Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,66 @@
1+
package internal
2+
3+
import (
4+
"context"
5+
"os"
6+
"os/exec"
7+
"time"
8+
)
9+
10+
// WaitOrStop waits for the already-started command cmd by calling its Wait method.
11+
//
12+
// If cmd does not return before ctx is done, WaitOrStop sends it the given interrupt signal.
13+
// If killDelay is positive, WaitOrStop waits that additional period for Wait to return before sending os.Kill.
14+
func WaitOrStop(ctx context.Context, cmd *exec.Cmd, interrupt os.Signal, killDelay time.Duration) error {
15+
if cmd.Process == nil {
16+
panic("WaitOrStop called with a nil cmd.Process — missing Start call?")
17+
}
18+
if interrupt == nil {
19+
panic("WaitOrStop requires a non-nil interrupt signal")
20+
}
21+
22+
errc := make(chan error)
23+
go func() {
24+
select {
25+
case errc <- nil:
26+
return
27+
case <-ctx.Done():
28+
}
29+
30+
err := cmd.Process.Signal(interrupt)
31+
if err == nil {
32+
err = ctx.Err() // Report ctx.Err() as the reason we interrupted.
33+
} else if err.Error() == "os: process already finished" {
34+
errc <- nil
35+
return
36+
}
37+
38+
if killDelay > 0 {
39+
timer := time.NewTimer(killDelay)
40+
select {
41+
// Report ctx.Err() as the reason we interrupted the process...
42+
case errc <- ctx.Err():
43+
timer.Stop()
44+
return
45+
// ...but after killDelay has elapsed, fall back to a stronger signal.
46+
case <-timer.C:
47+
}
48+
49+
// Wait still hasn't returned.
50+
// Kill the process harder to make sure that it exits.
51+
//
52+
// Ignore any error: if cmd.Process has already terminated, we still
53+
// want to send ctx.Err() (or the error from the Interrupt call)
54+
// to properly attribute the signal that may have terminated it.
55+
_ = cmd.Process.Kill()
56+
}
57+
58+
errc <- err
59+
}()
60+
61+
waitErr := cmd.Wait()
62+
if interruptErr := <-errc; interruptErr != nil {
63+
return interruptErr
64+
}
65+
return waitErr
66+
}

‎sandbox.go‎

Lines changed: 43 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,7 @@ import (
3636

3737
"cloud.google.com/go/compute/metadata"
3838
"github.com/bradfitz/gomemcache/memcache"
39+
"golang.org/x/playground/internal"
3940
"golang.org/x/playground/internal/gcpdial"
4041
"golang.org/x/playground/sandbox/sandboxtypes"
4142
)
@@ -123,6 +124,11 @@ func (s *server) commandHandler(cachePrefix string, cmdFunc func(context.Context
123124
http.Error(w, http.StatusText(http.StatusInternalServerError), http.StatusInternalServerError)
124125
return
125126
}
127+
if strings.Contains(resp.Errors, goBuildTimeoutError) {
128+
// TODO(golang.org/issue/38052) - This should be a http.StatusBadRequest, but the UI requires a 200 to parse the response.
129+
s.writeResponse(w, resp, http.StatusOK)
130+
return
131+
}
126132
for _, e := range nonCachingErrors {
127133
if strings.Contains(resp.Errors, e) {
128134
s.log.Errorf("cmdFunc compilation error: %q", resp.Errors)
@@ -147,16 +153,21 @@ func (s *server) commandHandler(cachePrefix string, cmdFunc func(context.Context
147153
}
148154
}
149155

150-
var buf bytes.Buffer
151-
if err := json.NewEncoder(&buf).Encode(resp); err != nil {
152-
s.log.Errorf("error encoding response: %v", err)
153-
http.Error(w, http.StatusText(http.StatusInternalServerError), http.StatusInternalServerError)
154-
return
155-
}
156-
if _, err := io.Copy(w, &buf); err != nil {
157-
s.log.Errorf("io.Copy(w, &buf): %v", err)
158-
return
159-
}
156+
s.writeResponse(w, resp, http.StatusOK)
157+
}
158+
}
159+
160+
func (s *server) writeResponse(w http.ResponseWriter, resp *response, status int) {
161+
var buf bytes.Buffer
162+
if err := json.NewEncoder(&buf).Encode(resp); err != nil {
163+
s.log.Errorf("error encoding response: %v", err)
164+
http.Error(w, http.StatusText(http.StatusInternalServerError), http.StatusInternalServerError)
165+
return
166+
}
167+
w.WriteHeader(status)
168+
if _, err := io.Copy(w, &buf); err != nil {
169+
s.log.Errorf("io.Copy(w, &buf): %v", err)
170+
return
160171
}
161172
}
162173

@@ -450,9 +461,7 @@ func sandboxBuild(ctx context.Context, tmpDir string, in []byte, vet bool) (*bui
450461
br.exePath = filepath.Join(tmpDir, "a.out")
451462
goCache := filepath.Join(tmpDir, "gocache")
452463

453-
ctx, cancel := context.WithTimeout(ctx, maxCompileTime)
454-
defer cancel()
455-
cmd := exec.CommandContext(ctx, "/usr/local/go-faketime/bin/go", "build", "-o", br.exePath, "-tags=faketime")
464+
cmd := exec.Command("/usr/local/go-faketime/bin/go", "build", "-o", br.exePath, "-tags=faketime")
456465
cmd.Dir = tmpDir
457466
cmd.Env = []string{"GOOS=linux", "GOARCH=amd64", "GOROOT=/usr/local/go-faketime"}
458467
cmd.Env = append(cmd.Env, "GOCACHE="+goCache)
@@ -472,25 +481,30 @@ func sandboxBuild(ctx context.Context, tmpDir string, in []byte, vet bool) (*bui
472481
}
473482
cmd.Args = append(cmd.Args, buildPkgArg)
474483
cmd.Env = append(cmd.Env, "GOPATH="+br.goPath)
475-
t0 := time.Now()
476-
if out, err := cmd.CombinedOutput(); err != nil {
477-
if ctx.Err() == context.DeadlineExceeded {
478-
log.Printf("go build timed out after %v", time.Since(t0))
479-
return &buildResult{errorMessage: goBuildTimeoutError}, nil
480-
}
481-
if _, ok := err.(*exec.ExitError); ok {
482-
// Return compile errors to the user.
484+
out := &bytes.Buffer{}
485+
cmd.Stderr, cmd.Stdout = out, out
483486

484-
// Rewrite compiler errors to strip the tmpDir name.
485-
br.errorMessage = strings.Replace(string(out), tmpDir+"/", "", -1)
487+
if err := cmd.Start(); err != nil {
488+
return nil, fmt.Errorf("error starting go build: %v", err)
489+
}
490+
ctx, cancel := context.WithTimeout(ctx, maxCompileTime)
491+
defer cancel()
492+
if err := internal.WaitOrStop(ctx, cmd, os.Interrupt, 250*time.Millisecond); err != nil {
493+
if errors.Is(err, context.DeadlineExceeded) {
494+
br.errorMessage = fmt.Sprintln(goBuildTimeoutError)
495+
} else if ee := (*exec.ExitError)(nil); !errors.As(err, &ee) {
496+
log.Printf("error building program: %v", err)
497+
return nil, fmt.Errorf("error building go source: %v", err)
498+
}
499+
// Return compile errors to the user.
500+
// Rewrite compiler errors to strip the tmpDir name.
501+
br.errorMessage = br.errorMessage + strings.Replace(string(out.Bytes()), tmpDir+"/", "", -1)
486502

487-
// "go build", invoked with a file name, puts this odd
488-
// message before any compile errors; strip it.
489-
br.errorMessage = strings.Replace(br.errorMessage, "# command-line-arguments\n", "", 1)
503+
// "go build", invoked with a file name, puts this odd
504+
// message before any compile errors; strip it.
505+
br.errorMessage = strings.Replace(br.errorMessage, "# command-line-arguments\n", "", 1)
490506

491-
return br, nil
492-
}
493-
return nil, fmt.Errorf("error building go source: %v", err)
507+
return br, nil
494508
}
495509
const maxBinarySize = 100 << 20 // copied from sandbox backend; TODO: unify?
496510
if fi, err := os.Stat(br.exePath); err != nil || fi.Size() == 0 || fi.Size() > maxBinarySize {

‎sandbox/sandbox.go‎

Lines changed: 44 additions & 55 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@ import (
3030
"syscall"
3131
"time"
3232

33+
"golang.org/x/playground/internal"
3334
"golang.org/x/playground/sandbox/sandboxtypes"
3435
)
3536

@@ -70,34 +71,31 @@ var (
7071
)
7172

7273
type Container struct {
73-
name string
74+
name string
75+
7476
stdin io.WriteCloser
7577
stdout *limitedWriter
7678
stderr *limitedWriter
77-
cmd *exec.Cmd
7879

79-
waitOnce sync.Once
80-
waitVal error
80+
cmd *exec.Cmd
81+
cancelCmd context.CancelFunc
82+
83+
waitErr chan error // 1-buffered; receives error from WaitOrStop(..., cmd, ...)
8184
}
8285

8386
func (c *Container) Close() {
8487
setContainerWanted(c.name, false)
8588

86-
if c.cmd.Process != nil {
87-
gracefulStop(c.cmd.Process, 250*time.Millisecond)
88-
if err := c.Wait(); err != nil {
89-
log.Printf("error in c.Wait() for %q: %v", c.name, err)
90-
}
89+
c.cancelCmd()
90+
if err := c.Wait(); err != nil {
91+
log.Printf("error in c.Wait() for %q: %v", c.name, err)
9192
}
9293
}
9394

9495
func (c *Container) Wait() error {
95-
c.waitOnce.Do(c.wait)
96-
return c.waitVal
97-
}
98-
99-
func (c *Container) wait() {
100-
c.waitVal = c.cmd.Wait()
96+
err := <-c.waitErr
97+
c.waitErr <- err
98+
return err
10199
}
102100

103101
var httpServer *http.Server
@@ -267,34 +265,17 @@ func runInGvisor() {
267265
if err := cmd.Start(); err != nil {
268266
log.Fatalf("cmd.Start(): %v", err)
269267
}
270-
timer := time.AfterFunc(runTimeout-(500*time.Millisecond), func() {
271-
fmt.Fprintln(os.Stderr, "timeout running program")
272-
gracefulStop(cmd.Process, 250*time.Millisecond)
273-
})
274-
defer timer.Stop()
275-
err = cmd.Wait()
268+
ctx, cancel := context.WithTimeout(context.Background(), runTimeout-500*time.Millisecond)
269+
defer cancel()
270+
if err = internal.WaitOrStop(ctx, cmd, os.Interrupt, 250*time.Millisecond); err != nil {
271+
if errors.Is(err, context.DeadlineExceeded) {
272+
fmt.Fprintln(os.Stderr, "timeout running program")
273+
}
274+
}
276275
os.Exit(errExitCode(err))
277276
return
278277
}
279278

280-
// gracefulStop attempts to send a SIGINT before a SIGKILL.
281-
//
282-
// The process will be sent a SIGINT immediately. If the context has still not been cancelled,
283-
// the process will be sent a SIGKILL after delay has passed since sending the SIGINT.
284-
//
285-
// TODO(golang.org/issue/38343) - Change SIGINT to SIGQUIT once decision is made.
286-
func gracefulStop(p *os.Process, delay time.Duration) {
287-
// TODO(golang.org/issue/38343) - Change to syscall.SIGQUIT once decision is made.
288-
if err := p.Signal(os.Interrupt); err != nil {
289-
log.Printf("cmd.Process.Signal(%v): %v", os.Interrupt, err)
290-
}
291-
time.AfterFunc(delay, func() {
292-
if err := p.Kill(); err != nil {
293-
log.Printf("cmd.Process.Kill(): %v", err)
294-
}
295-
})
296-
}
297-
298279
func makeWorkers() {
299280
for {
300281
c, err := startContainer(context.Background())
@@ -372,15 +353,25 @@ func startContainer(ctx context.Context) (c *Container, err error) {
372353
if err := cmd.Start(); err != nil {
373354
return nil, err
374355
}
356+
357+
ctx, cancel := context.WithCancel(ctx)
358+
c = &Container{
359+
name: name,
360+
stdin: stdin,
361+
stdout: stdout,
362+
stderr: stderr,
363+
cmd: cmd,
364+
cancelCmd: cancel,
365+
waitErr: make(chan error, 1),
366+
}
367+
go func() {
368+
c.waitErr <- internal.WaitOrStop(ctx, cmd, os.Interrupt, 250*time.Millisecond)
369+
}()
375370
defer func() {
376371
if err != nil {
377-
log.Printf("error starting container %q: %v", name, err)
378-
gracefulStop(cmd.Process, 250*time.Millisecond)
379-
setContainerWanted(name, false)
372+
c.Close()
380373
}
381374
}()
382-
ctx, cancel := context.WithTimeout(ctx, startTimeout)
383-
defer cancel()
384375

385376
startErr := make(chan error, 1)
386377
go func() {
@@ -395,25 +386,23 @@ func startContainer(ctx context.Context) (c *Container, err error) {
395386
}
396387
}()
397388

389+
timer := time.NewTimer(startTimeout)
390+
defer timer.Stop()
398391
select {
399-
case <-ctx.Done():
400-
err := fmt.Errorf("timeout starting container %q: %w", name, ctx.Err())
401-
pw.Close()
392+
case <-timer.C:
393+
err := fmt.Errorf("timeout starting container %q", name)
394+
cancel()
402395
<-startErr
403396
return nil, err
404-
case err = <-startErr:
397+
398+
case err := <-startErr:
405399
if err != nil {
406400
return nil, err
407401
}
408402
}
403+
409404
log.Printf("started container %q", name)
410-
return &Container{
411-
name: name,
412-
stdin: stdin,
413-
stdout: stdout,
414-
stderr: stderr,
415-
cmd: cmd,
416-
}, nil
405+
return c, nil
417406
}
418407

419408
func runHandler(w http.ResponseWriter, r *http.Request) {

0 commit comments

Comments
 (0)