Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 7 commits into from Sep 18, 2020

Conversation

hongalex
Copy link
Member

In #2624, we split the publisher and subscriber client's connection pool. Thus, we need to check both errors returned, rather than just the first one.

@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Sep 15, 2020
Copy link
Contributor

@shollyman shollyman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving, but please see the comment about close semantics and address if necessary.

pubsub/pubsub.go Outdated Show resolved Hide resolved
@product-auto-label product-auto-label bot added the api: pubsub Issues related to the Pub/Sub API. label Sep 16, 2020
pubsub/pubsub.go Show resolved Hide resolved
pubsub/streaming_pull_test.go Outdated Show resolved Hide resolved
@hongalex hongalex requested a review from tbpg September 18, 2020 18:40
@hongalex hongalex merged commit 07df7d8 into googleapis:master Sep 18, 2020
@hongalex hongalex deleted the pubsub-close-error branch September 18, 2020 22:35
@xmlking
Copy link

xmlking commented Oct 1, 2020

@hongalex I was debugging why I am getting "pubsub publisher closing error: rpc error: code = Canceled desc = grpc: the client connection is closing error when called client.Close()
After upgrading to cloud.google.com/go/pubsub v1.8.0 I started see this error. I never got this error in previous pubsub versions.

I accidentally discovered there is a type in the error message at:
https://github.com/googleapis/google-cloud-go/blob/master/pubsub/pubsub.go#L100-L109

this should be:

func (c *Client) Close() error {
	pubErr := c.pubc.Close()
	subErr := c.subc.Close()
	if pubErr != nil {
                 return fmt.Errorf("pubsub publisher closing error: %v", pubErr)
	}
	if subErr != nil {
		return fmt.Errorf("pubsub subscriber closing error: %v", subErr)
	}
	return nil
}

@xmlking
Copy link

xmlking commented Oct 1, 2020

Should we detect PubSub emulator environment and automatically suppress the client connection is closing error ?
otherwise this leads to confusion to users

@hongalex
Copy link
Member Author

hongalex commented Oct 1, 2020

Hey @xmlking, this error does occur with the emulator and the pstest fake server. I'll address this in a separate PR so we can suppress "the client connection is closing" errors. Also, thanks for catching the typo.

Btw, I noticed in the linked issue, there's a comment that says

Live pubsub server will throw this error

Though I think the opposite is true (live pub/sub service will not throw this error).

@xmlking
Copy link

xmlking commented Oct 1, 2020

thanks @hongalex . fixed my type

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: pubsub Issues related to the Pub/Sub API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants