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

eventbus: dont panic on closing Subscription twice #3034

Merged
merged 1 commit into from
Nov 13, 2024

Conversation

sukunrt
Copy link
Member

@sukunrt sukunrt commented Nov 11, 2024

fixes: #3002

@sukunrt sukunrt requested a review from MarcoPolo November 11, 2024 15:22
@MarcoPolo
Copy link
Collaborator

I'm not sure. Why wouldn't this follow the same semantics as closing a channel twice? https://play.golang.com/p/S5frqwAAI1j. That panics, and this seems similar to me.

@sukunrt
Copy link
Member Author

sukunrt commented Nov 12, 2024

Not a fan of that decision either.

Channel closing twice panics so that it explicitly disallows broken concurrency primitives and avoids race conditions with multiple senders closing the channel.

In our case, there's no way to synchronize a subscriber with an emitter. In fact, when dealing with channels, the receiver never closes a channel, it's always the sender who closes the channel because otherwise the sender might panic. Here the subscriber closes the channel.

Plus, In almost all cases, the subscriber is closed at the end of the object life time. It seems fine to close it twice with the second one being a no op just like calling Close on a stream twice isn't a panic.

Copy link
Collaborator

@MarcoPolo MarcoPolo left a comment

Choose a reason for hiding this comment

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

I'm still not convinced by the argument, but I really don't like panics.

@sukunrt
Copy link
Member Author

sukunrt commented Nov 13, 2024

I don't like them either!

@sukunrt sukunrt merged commit 7268c98 into master Nov 13, 2024
10 of 11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Issue panic when user interrupt signal
2 participants