go/internal/packagesdriver: be defensive wrt error results

While investigating a bug that turned out to lie elsewhere
(#63700), we were troubled by the inconsistencies between
the pair of errors returned by functions in this package.

This change makes things more consistent.

Updates golang/go#63700

Change-Id: I926c47572b7f666327bd1dba71ace68a5591bf2f
Reviewed-on: https://go-review.googlesource.com/c/tools/+/537875
Reviewed-by: Robert Findley <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>

Author: adonovan

go/internal/packagesdriver/sizes.go

@@ -13,16 +13,17 @@ import (
 	"golang.org/x/tools/internal/gocommand"
 )
 
-var debug = false
-
 func GetSizesForArgsGolist(ctx context.Context, inv gocommand.Invocation, gocmdRunner *gocommand.Runner) (string, string, error) {
 	inv.Verb = "list"
 	inv.Args = []string{"-f", "{{context.GOARCH}} {{context.Compiler}}", "--", "unsafe"}
 	stdout, stderr, friendlyErr, rawErr := gocmdRunner.RunRaw(ctx, inv)
 	var goarch, compiler string
 	if rawErr != nil {
-		if rawErrMsg := rawErr.Error(); strings.Contains(rawErrMsg, "cannot find main module") || strings.Contains(rawErrMsg, "go.mod file not found") {
-			// User's running outside of a module. All bets are off. Get GOARCH and guess compiler is gc.
+		rawErrMsg := rawErr.Error()
+		if strings.Contains(rawErrMsg, "cannot find main module") ||
+			strings.Contains(rawErrMsg, "go.mod file not found") {
+			// User's running outside of a module.
+			// All bets are off. Get GOARCH and guess compiler is gc.
 			// TODO(matloob): Is this a problem in practice?
 			inv.Verb = "env"
 			inv.Args = []string{"GOARCH"}
@@ -32,8 +33,12 @@ func GetSizesForArgsGolist(ctx context.Context, inv gocommand.Invocation, gocmdR
 			}
 			goarch = strings.TrimSpace(envout.String())
 			compiler = "gc"
-		} else {
+		} else if friendlyErr != nil {
 			return "", "", friendlyErr
+		} else {
+			// This should be unreachable, but be defensive
+			// in case RunRaw's error results are inconsistent.
+			return "", "", rawErr
 		}
 	} else {
 		fields := strings.Fields(stdout.String())

internal/gocommand/invoke.go

@@ -85,6 +85,7 @@ func (runner *Runner) RunPiped(ctx context.Context, inv Invocation, stdout, stde
 
 // RunRaw runs the invocation, serializing requests only if they fight over
 // go.mod changes.
+// Postcondition: both error results have same nilness.
 func (runner *Runner) RunRaw(ctx context.Context, inv Invocation) (*bytes.Buffer, *bytes.Buffer, error, error) {
 	ctx, done := event.Start(ctx, "gocommand.Runner.RunRaw", invLabels(inv)...)
 	defer done()
@@ -95,23 +96,24 @@ func (runner *Runner) RunRaw(ctx context.Context, inv Invocation) (*bytes.Buffer
 	stdout, stderr, friendlyErr, err := runner.runConcurrent(ctx, inv)
 
 	// If we encounter a load concurrency error, we need to retry serially.
-	if friendlyErr == nil || !modConcurrencyError.MatchString(friendlyErr.Error()) {
-		return stdout, stderr, friendlyErr, err
+	if friendlyErr != nil && modConcurrencyError.MatchString(friendlyErr.Error()) {
+		event.Error(ctx, "Load concurrency error, will retry serially", err)
+
+		// Run serially by calling runPiped.
+		stdout.Reset()
+		stderr.Reset()
+		friendlyErr, err = runner.runPiped(ctx, inv, stdout, stderr)
 	}
-	event.Error(ctx, "Load concurrency error, will retry serially", err)
 
-	// Run serially by calling runPiped.
-	stdout.Reset()
-	stderr.Reset()
-	friendlyErr, err = runner.runPiped(ctx, inv, stdout, stderr)
 	return stdout, stderr, friendlyErr, err
 }
 
+// Postcondition: both error results have same nilness.
 func (runner *Runner) runConcurrent(ctx context.Context, inv Invocation) (*bytes.Buffer, *bytes.Buffer, error, error) {
 	// Wait for 1 worker to become available.
 	select {
 	case <-ctx.Done():
-		return nil, nil, nil, ctx.Err()
+		return nil, nil, ctx.Err(), ctx.Err()
 	case runner.inFlight <- struct{}{}:
 		defer func() { <-runner.inFlight }()
 	}
@@ -121,6 +123,7 @@ func (runner *Runner) runConcurrent(ctx context.Context, inv Invocation) (*bytes
 	return stdout, stderr, friendlyErr, err
 }
 
+// Postcondition: both error results have same nilness.
 func (runner *Runner) runPiped(ctx context.Context, inv Invocation, stdout, stderr io.Writer) (error, error) {
 	// Make sure the runner is always initialized.
 	runner.initialize()
@@ -129,7 +132,7 @@ func (runner *Runner) runPiped(ctx context.Context, inv Invocation, stdout, stde
 	// runPiped commands.
 	select {
 	case <-ctx.Done():
-		return nil, ctx.Err()
+		return ctx.Err(), ctx.Err()
 	case runner.serialized <- struct{}{}:
 		defer func() { <-runner.serialized }()
 	}
@@ -139,7 +142,7 @@ func (runner *Runner) runPiped(ctx context.Context, inv Invocation, stdout, stde
 	for i := 0; i < maxInFlight; i++ {
 		select {
 		case <-ctx.Done():
-			return nil, ctx.Err()
+			return ctx.Err(), ctx.Err()
 		case runner.inFlight <- struct{}{}:
 			// Make sure we always "return" any workers we took.
 			defer func() { <-runner.inFlight }()
@@ -172,6 +175,7 @@ type Invocation struct {
 	Logf       func(format string, args ...interface{})
 }
 
+// Postcondition: both error results have same nilness.
 func (i *Invocation) runWithFriendlyError(ctx context.Context, stdout, stderr io.Writer) (friendlyError error, rawError error) {
 	rawError = i.run(ctx, stdout, stderr)
 	if rawError != nil {