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

pstest: Topic.Delete stops subscription unnecessarily #1938

Closed
grantr opened this issue Apr 17, 2020 · 0 comments
Closed

pstest: Topic.Delete stops subscription unnecessarily #1938

grantr opened this issue Apr 17, 2020 · 0 comments
Assignees
Labels
api: pubsub Issues related to the Pub/Sub API. priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.

Comments

@grantr
Copy link
Contributor

grantr commented Apr 17, 2020

Client
pstest

Environment
Go test

Code

runnable gist

topic, _ := c.CreateTopic(ctx, "topic")
sub, _ := c.CreateSubscription(ctx, "sub", pubsub.SubscriptionConfig{Topic: topic})
topic.Delete(ctx)
sub.Delete(ctx)

Expected behavior
The topic and sub can be deleted in that order without an error.

Actual behavior

panic: close of closed channel

goroutine 52 [running]:
cloud.google.com/go/pubsub/pstest.(*subscription).stop(...)
	cloud.google.com/go/pubsub/pstest/fake.go:591
cloud.google.com/go/pubsub/pstest.(*GServer).DeleteSubscription(0xc0000be138, 0x16ef8e0, 0xc0004011a0, 0xc0004011d0, 0x0, 0x0, 0x0)
	cloud.google.com/go/pubsub/pstest/fake.go:470 +0xfd
google.golang.org/genproto/googleapis/pubsub/v1._Subscriber_DeleteSubscription_Handler(0x16086a0, 0xc0000be138, 0x16ef8e0, 0xc0004011a0, 0xc000486a80, 0x0, 0x16ef8e0, 0xc0004011a0, 0xc0004c4330, 0x24)
	google.golang.org/genproto/googleapis/pubsub/v1/pubsub.pb.go:3645 +0x217
google.golang.org/grpc.(*Server).processUnaryRPC(0xc0000d0f20, 0x16f48c0, 0xc0003a2480, 0xc0004c8600, 0xc000122d20, 0x1afec40, 0x0, 0x0, 0x0)
	google.golang.org/grpc/server.go:995 +0x460
google.golang.org/grpc.(*Server).handleStream(0xc0000d0f20, 0x16f48c0, 0xc0003a2480, 0xc0004c8600, 0x0)
	google.golang.org/grpc/server.go:1275 +0xd3d
google.golang.org/grpc.(*Server).serveStreams.func1.1(0xc00039c060, 0xc0000d0f20, 0x16f48c0, 0xc0003a2480, 0xc0004c8600)
	google.golang.org/grpc/server.go:710 +0xa1
created by google.golang.org/grpc.(*Server).serveStreams.func1
	google.golang.org/grpc/server.go:708 +0xa1

Additional context
The bug is here:

When a topic is stopped, its subscriptions are also stopped which closes the subscription's done channel. When the subscription is later stopped, there is no check that the done channel is already closed.

The behavior of stopping the subscription on topic delete is incorrect. See https://cloud.google.com/pubsub/docs/admin#deleting_a_topic:

When you delete a topic, its subscriptions are not deleted, and the subscription's message backlog is available for subscribers.

@grantr grantr added the triage me I really want to be triaged. label Apr 17, 2020
@IlyaFaer IlyaFaer added the api: pubsub Issues related to the Pub/Sub API. label Apr 20, 2020
@codyoss codyoss added priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. and removed triage me I really want to be triaged. labels Apr 20, 2020
@hongalex hongalex self-assigned this Apr 28, 2020
grantr added a commit to grantr/google-cloud-go that referenced this issue May 1, 2020
grantr added a commit to grantr/knative-gcp that referenced this issue May 6, 2020
This is a temporary replace until the fix for
googleapis/google-cloud-go#1938 is in a release.
knative-prow-robot pushed a commit to google/knative-gcp that referenced this issue May 8, 2020
* Use standard pubsub client in Broker controller

Broker controller no longer uses the gclient interface.

Tests now use the pstest fake pubsub server instead of the gclient mock.
Tests verify pubsub resource creation and deletion.

* Switch to the pubsub client in master

This is a temporary replace until the fix for
googleapis/google-cloud-go#1938 is in a release.

* Use a single pubsub client for all broker reconciles

If the pubsub client cannot be created immediately, the controller will leave
it up to the reconcilers to create one when needed.

* Correct copyright header
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. priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.
Projects
None yet
Development

No branches or pull requests

4 participants