Merge pull request #617 from grafana/fix/614-abort-non-fatal-error

Wait for browser process to exit before returning first error from stderr

Author: inancgumus

common/browser_process.go

@@ -179,41 +179,71 @@ func execute(
 	return command{cmd, done, stdout, stderr}, nil
 }
 
-// parseDevToolsURL grabs the websocket address from chrome's output and returns it.
-func parseDevToolsURL(ctx context.Context, cmd command) (wsURL string, _ error) {
-	type result struct {
-		devToolsURL string
-		err         error
+// parseDevToolsURL grabs the WebSocket address from Chrome's output and returns
+// it. If the process ends abruptly, it will return the first error from stderr.
+func parseDevToolsURL(ctx context.Context, cmd command) (_ string, err error) {
+	parser := &devToolsURLParser{
+		sc: bufio.NewScanner(cmd.stderr),
 	}
-	c := make(chan result, 1)
+	done := make(chan struct{})
 	go func() {
-		const urlPrefix = "DevTools listening on "
-		scanner := bufio.NewScanner(cmd.stderr)
-
-		for scanner.Scan() {
-			line := scanner.Text()
-			if strings.HasPrefix(line, urlPrefix) {
-				c <- result{
-					strings.TrimPrefix(strings.TrimSpace(line), urlPrefix),
-					nil,
-				}
-				return
-			}
-			if strings.Contains(line, ":ERROR:") {
-				if i := strings.Index(line, "] "); i > 0 {
-					c <- result{"", errors.New(line[i+2:])}
-					return
-				}
-			}
-		}
-		if err := scanner.Err(); err != nil {
-			c <- result{"", err}
+		for parser.scan() {
 		}
+		close(done)
 	}()
-	select {
-	case r := <-c:
-		return r.devToolsURL, r.err
-	case <-ctx.Done():
-		return "", fmt.Errorf("%w", ctx.Err())
+	for err == nil {
+		select {
+		case <-done:
+			err = parser.err()
+		case <-ctx.Done():
+			err = ctx.Err()
+		case <-cmd.done:
+			err = errors.New("browser process ended unexpectedly")
+		}
+	}
+	if parser.url != "" {
+		err = nil
+	}
+
+	return parser.url, err
+}
+
+type devToolsURLParser struct {
+	sc *bufio.Scanner
+
+	errs []error
+	url  string
+}
+
+func (p *devToolsURLParser) scan() bool {
+	if !p.sc.Scan() {
+		return false
+	}
+
+	const urlPrefix = "DevTools listening on "
+
+	line := p.sc.Text()
+	if strings.HasPrefix(line, urlPrefix) {
+		p.url = strings.TrimPrefix(strings.TrimSpace(line), urlPrefix)
+	}
+	if strings.Contains(line, ":ERROR:") {
+		if i := strings.Index(line, "] "); i > 0 {
+			p.errs = append(p.errs, errors.New(line[i+2:]))
+		}
+	}
+
+	return p.url == ""
+}
+
+func (p *devToolsURLParser) err() error {
+	if p.url != "" {
+		return io.EOF
+	}
+	if len(p.errs) > 0 {
+		return p.errs[0]
+	}
+	if err := p.sc.Err(); err != nil {
+		return fmt.Errorf("%w", err)
 	}
+	return nil
 }

common/browser_process_test.go

@@ -0,0 +1,187 @@
+package common
+
+import (
+	"context"
+	"io"
+	"testing"
+	"time"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+type mockCommand struct {
+	*command
+	cancelFn context.CancelFunc
+}
+
+type mockReader struct {
+	lines []string
+	hook  func()
+	err   error
+}
+
+func (r *mockReader) Read(p []byte) (n int, err error) {
+	if r.hook != nil {
+		// Allow some time for the read to be processed
+		time.AfterFunc(100*time.Millisecond, r.hook)
+		r.hook = nil // Ensure the hook only runs once
+	}
+	if len(r.lines) == 0 {
+		return 0, io.EOF
+	}
+	n = copy(p, []byte(r.lines[0]+"\n"))
+	r.lines = r.lines[1:]
+
+	return n, r.err
+}
+
+func TestParseDevToolsURL(t *testing.T) {
+	t.Parallel()
+
+	testCases := []struct {
+		name     string
+		stderr   []string
+		readErr  error
+		readHook func(c *mockCommand)
+		assert   func(t *testing.T, wsURL string, err error)
+	}{
+		{
+			name: "ok/no_error",
+			stderr: []string{
+				`DevTools listening on ws://127.0.0.1:41315/devtools/browser/d1d3f8eb-b362-4f12-9370-bd25778d0da7`,
+			},
+			assert: func(t *testing.T, wsURL string, err error) {
+				t.Helper()
+				require.NoError(t, err)
+				assert.Equal(t, "ws://127.0.0.1:41315/devtools/browser/d1d3f8eb-b362-4f12-9370-bd25778d0da7", wsURL)
+			},
+		},
+		{
+			name: "ok/non-fatal_error",
+			stderr: []string{
+				`[23400:23418:1028/115455.877614:ERROR:bus.cc(399)] Failed to ` +
+					`connect to the bus: Could not parse server address: ` +
+					`Unknown address type (examples of valid types are "tcp" ` +
+					`and on UNIX "unix")`,
+				"",
+				`DevTools listening on ws://127.0.0.1:41315/devtools/browser/d1d3f8eb-b362-4f12-9370-bd25778d0da7`,
+			},
+			assert: func(t *testing.T, wsURL string, err error) {
+				t.Helper()
+				require.NoError(t, err)
+				assert.Equal(t, "ws://127.0.0.1:41315/devtools/browser/d1d3f8eb-b362-4f12-9370-bd25778d0da7", wsURL)
+			},
+		},
+		{
+			name: "err/fatal-eof",
+			stderr: []string{
+				`[6497:6497:1013/103521.932979:ERROR:ozone_platform_x11` +
+					`.cc(247)] Missing X server or $DISPLAY` + "\n",
+			},
+			readErr: io.ErrUnexpectedEOF,
+			assert: func(t *testing.T, wsURL string, err error) {
+				t.Helper()
+				require.Empty(t, wsURL)
+				assert.EqualError(t, err, "Missing X server or $DISPLAY")
+			},
+		},
+		{
+			name:    "err/fatal-eof-no_stderr",
+			stderr:  []string{""},
+			readErr: io.ErrUnexpectedEOF,
+			assert: func(t *testing.T, wsURL string, err error) {
+				t.Helper()
+				require.Empty(t, wsURL)
+				assert.EqualError(t, err, "unexpected EOF")
+			},
+		},
+		{
+			// Ensure any error found on stderr is returned first.
+			name: "err/fatal-premature_cmd_done-stderr",
+			stderr: []string{
+				`[6497:6497:1013/103521.932979:ERROR:ozone_platform_x11` +
+					`.cc(247)] Missing X server or $DISPLAY` + "\n",
+			},
+			readHook: func(c *mockCommand) { close(c.done) },
+			assert: func(t *testing.T, wsURL string, err error) {
+				t.Helper()
+				require.Empty(t, wsURL)
+				assert.EqualError(t, err, "Missing X server or $DISPLAY")
+			},
+		},
+		{
+			// If there's no error on stderr, return a generic error.
+			name:     "err/fatal-premature_cmd_done-no_stderr",
+			stderr:   []string{""},
+			readHook: func(c *mockCommand) { close(c.done) },
+			assert: func(t *testing.T, wsURL string, err error) {
+				t.Helper()
+				require.Empty(t, wsURL)
+				assert.EqualError(t, err, "browser process ended unexpectedly")
+			},
+		},
+		{
+			name: "err/fatal-premature_ctx_cancel-stderr",
+			stderr: []string{
+				`[6497:6497:1013/103521.932979:ERROR:ozone_platform_x11` +
+					`.cc(247)] Missing X server or $DISPLAY` + "\n",
+			},
+			readHook: func(c *mockCommand) { c.cancelFn() },
+			assert: func(t *testing.T, wsURL string, err error) {
+				t.Helper()
+				require.Empty(t, wsURL)
+				assert.EqualError(t, err, "Missing X server or $DISPLAY")
+			},
+		},
+		{
+			name:     "err/fatal-premature_ctx_cancel-no_stderr",
+			stderr:   []string{""},
+			readHook: func(c *mockCommand) { c.cancelFn() },
+			assert: func(t *testing.T, wsURL string, err error) {
+				t.Helper()
+				require.Empty(t, wsURL)
+				assert.EqualError(t, err, "context canceled")
+			},
+		},
+	}
+
+	for _, tc := range testCases {
+		tc := tc
+		t.Run(tc.name, func(t *testing.T) {
+			t.Parallel()
+
+			ctx, cancel := context.WithCancel(context.Background())
+			t.Cleanup(cancel)
+
+			var cmd *mockCommand
+			mr := mockReader{lines: tc.stderr, err: tc.readErr}
+			if tc.readHook != nil {
+				mr.hook = func() { tc.readHook(cmd) }
+			}
+			cmd = &mockCommand{&command{done: make(chan struct{}), stderr: &mr}, cancel}
+
+			timeout := time.Second
+			timer := time.NewTimer(timeout)
+			t.Cleanup(func() { _ = timer.Stop() })
+
+			var (
+				done  = make(chan struct{})
+				wsURL string
+				err   error
+			)
+
+			go func() {
+				wsURL, err = parseDevToolsURL(ctx, *cmd.command)
+				close(done)
+			}()
+
+			select {
+			case <-done:
+				tc.assert(t, wsURL, err)
+			case <-timer.C:
+				t.Errorf("test timed out after %s", timeout)
+			}
+		})
+	}
+}