fix(pubsub): check both client errors on Close (#2867)

* fix(pubsub): check both client errors on Close

* fix code formatting

* don't check error from client.Close

* call both Close before returning error

* address review comments

Author: hongalex

pubsub/go.sum

@@ -433,6 +433,7 @@ google.golang.org/protobuf v1.25.0/go.mod h1:9JNX74DMeImyA3h4bdi1ymwjUzf21/xIlba
 gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0=
 gopkg.in/check.v1 v1.0.0-20180628173108-788fd7840127/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0=
 gopkg.in/errgo.v2 v2.1.0/go.mod h1:hNsd1EY+bozCKY1Ytp96fpM3vjJbqLJn88ws8XvfDNI=
+gopkg.in/yaml.v2 v2.2.2 h1:ZCJp+EgiOT7lHqUV2J862kp8Qj64Jo6az82+3Td9dZw=
 gopkg.in/yaml.v2 v2.2.2/go.mod h1:hI93XBmqTisBFMUTm0b8Fm+jr3Dg1NNxqwp+5A1VGuI=
 honnef.co/go/tools v0.0.0-20190102054323-c2f93a96b099/go.mod h1:rf3lG4BRIbNafJWhAfAdb/ePZxsR/4RtNHQocxwk9r4=
 honnef.co/go/tools v0.0.0-20190106161140-3f1c8253044a/go.mod h1:rf3lG4BRIbNafJWhAfAdb/ePZxsR/4RtNHQocxwk9r4=

pubsub/pubsub.go

@@ -78,14 +78,11 @@ func NewClient(ctx context.Context, projectID string, opts ...option.ClientOptio
 	o = append(o, opts...)
 	pubc, err := vkit.NewPublisherClient(ctx, o...)
 	if err != nil {
-		return nil, fmt.Errorf("pubsub: %v", err)
+		return nil, fmt.Errorf("pubsub(publisher): %v", err)
 	}
 	subc, err := vkit.NewSubscriberClient(ctx, o...)
 	if err != nil {
-		// Should never happen, since we are passing in the connection.
-		// If it does, we cannot close, because the user may have passed in their
-		// own connection originally.
-		return nil, fmt.Errorf("pubsub: %v", err)
+		return nil, fmt.Errorf("pubsub(subscriber): %v", err)
 	}
 	pubc.SetGoogleClientInfo("gccl", version.Repo)
 	return &Client{
@@ -101,10 +98,15 @@ func NewClient(ctx context.Context, projectID string, opts ...option.ClientOptio
 // If the client is available for the lifetime of the program, then Close need not be
 // called at exit.
 func (c *Client) Close() error {
-	// Return the first error, because the first call closes the connection.
-	err := c.pubc.Close()
-	_ = c.subc.Close()
-	return err
+	pubErr := c.pubc.Close()
+	subErr := c.subc.Close()
+	if pubErr != nil {
+		return fmt.Errorf("pubsub subscriber closing error: %v", pubErr)
+	}
+	if subErr != nil {
+		return fmt.Errorf("pubsub publisher closing error: %v", subErr)
+	}
+	return nil
 }
 
 func (c *Client) fullyQualifiedProjectName() string {

pubsub/streaming_pull_test.go

@@ -315,10 +315,12 @@ func TestStreamingPull_ClosedClient(t *testing.T) {
 	// wait for receives to happen
 	time.Sleep(100 * time.Millisecond)
 
-	err := client.Close()
-	if err != nil {
-		t.Fatal(err)
-	}
+	// Intentionally don't check the returned err here. In the fake,
+	// closing either the publisher/subscriber client will cause the
+	// server to clean up resources, which is different than in the
+	// live service. With the fake, client.Close() will return a
+	// "the client connection is closing" error with the second Close.
+	client.Close()
 
 	// wait for things to close
 	time.Sleep(100 * time.Millisecond)
@@ -327,7 +329,7 @@ func TestStreamingPull_ClosedClient(t *testing.T) {
 	case recvErr := <-recvFinished:
 		s, ok := status.FromError(recvErr)
 		if !ok {
-			t.Fatalf("Expected a gRPC failure, got %v", err)
+			t.Fatalf("Expected a gRPC failure, got %v", recvErr)
 		}
 		if s.Code() != codes.Canceled {
 			t.Fatalf("Expected canceled, got %v", s.Code())