bigtable: fix error variable shadowing bugs when tracing by named return

Previously in bigtable.(*Table).Apply we started a span
and on defer ended with an error that had been previously
declared. However, in the body of that function, while
in the m.cond == nil scope, we shadowed the original error with:

    err := gax.Invoke

which means that regardless of the result of that newly
declared/shadowed "err" error, we would always end the span
with a success.

An illustration of this problem is:

  https://play.golang.org/p/1iesjtxGI-e

Using a named return parameter ensures that we always
capture the finally written error value, regardless of
shadowing or a new name at return time.

While here I examined a few usages of
func <func_name>(ctx context.Context) error {
    var err error
    ctx = trace.Startspan(ctx, <span_name>)
    defer func() { trace.EndSpan(ctx, err) }()

and changed them to:
func <func_name>(ctx context.Context) (err error) {
    ctx = trace.Startspan(ctx, <span_name>)
    defer func() { trace.EndSpan(ctx, err) }()

which will prevent the reported problem from creeping back in.

Change-Id: I63dd12486844b2fae9d946f895e5c85f80a3a3e1
Reviewed-on: https://code-review.googlesource.com/c/gocloud/+/39651
Reviewed-by: kokoro <[email protected]>
Reviewed-by: Jean de Klerk <[email protected]>

Author: odeke-em

bigtable/bigtable.go

@@ -143,13 +143,12 @@ func (c *Client) Open(table string) *Table {
 //
 // By default, the yielded rows will contain all values in all cells.
 // Use RowFilter to limit the cells returned.
-func (t *Table) ReadRows(ctx context.Context, arg RowSet, f func(Row) bool, opts ...ReadOption) error {
+func (t *Table) ReadRows(ctx context.Context, arg RowSet, f func(Row) bool, opts ...ReadOption) (err error) {
 	ctx = mergeOutgoingMetadata(ctx, t.md)
-
-	var prevRowKey string
-	var err error
 	ctx = trace.StartSpan(ctx, "cloud.google.com/go/bigtable.ReadRows")
 	defer func() { trace.EndSpan(ctx, err) }()
+
+	var prevRowKey string
 	attrMap := make(map[string]interface{})
 	err = gax.Invoke(ctx, func(ctx context.Context) error {
 		if !arg.valid() {
@@ -469,15 +468,14 @@ const maxMutations = 100000
 
 // Apply mutates a row atomically. A mutation must contain at least one
 // operation and at most 100000 operations.
-func (t *Table) Apply(ctx context.Context, row string, m *Mutation, opts ...ApplyOption) error {
+func (t *Table) Apply(ctx context.Context, row string, m *Mutation, opts ...ApplyOption) (err error) {
 	ctx = mergeOutgoingMetadata(ctx, t.md)
 	after := func(res proto.Message) {
 		for _, o := range opts {
 			o.after(res)
 		}
 	}
 
-	var err error
 	ctx = trace.StartSpan(ctx, "cloud.google.com/go/bigtable/Apply")
 	defer func() { trace.EndSpan(ctx, err) }()
 	var callOptions []gax.CallOption
@@ -642,8 +640,11 @@ type entryErr struct {
 // will correspond to the relevant rowKeys/muts arguments.
 //
 // Conditional mutations cannot be applied in bulk and providing one will result in an error.
-func (t *Table) ApplyBulk(ctx context.Context, rowKeys []string, muts []*Mutation, opts ...ApplyOption) ([]error, error) {
+func (t *Table) ApplyBulk(ctx context.Context, rowKeys []string, muts []*Mutation, opts ...ApplyOption) (errs []error, err error) {
 	ctx = mergeOutgoingMetadata(ctx, t.md)
+	ctx = trace.StartSpan(ctx, "cloud.google.com/go/bigtable/ApplyBulk")
+	defer func() { trace.EndSpan(ctx, err) }()
+
 	if len(rowKeys) != len(muts) {
 		return nil, fmt.Errorf("mismatched rowKeys and mutation array lengths: %d, %d", len(rowKeys), len(muts))
 	}
@@ -657,10 +658,6 @@ func (t *Table) ApplyBulk(ctx context.Context, rowKeys []string, muts []*Mutatio
 		origEntries[i] = &entryErr{Entry: &btpb.MutateRowsRequest_Entry{RowKey: []byte(key), Mutations: mut.ops}}
 	}
 
-	var err error
-	ctx = trace.StartSpan(ctx, "cloud.google.com/go/bigtable/ApplyBulk")
-	defer func() { trace.EndSpan(ctx, err) }()
-
 	for _, group := range groupEntries(origEntries, maxMutations) {
 		attrMap := make(map[string]interface{})
 		err = gax.Invoke(ctx, func(ctx context.Context) error {
@@ -684,9 +681,8 @@ func (t *Table) ApplyBulk(ctx context.Context, rowKeys []string, muts []*Mutatio
 		}
 	}
 
-	// Accumulate all of the errors into an array to return, interspersed with nils for successful
+	// All the errors are accumulated into an array and returned, interspersed with nils for successful
 	// entries. The absence of any errors means we should return nil.
-	var errs []error
 	var foundErr bool
 	for _, entry := range origEntries {
 		if entry.Err != nil {