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 panic on closed channel #13

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

andpago
Copy link

@andpago andpago commented Oct 19, 2020

Fix panic in subscribers

When hub publishes messages, it first finds all matching subscribers and then calls sub.Set(m) on them:

// Publish will send an event to all the subscribers matching the event name.
func (h *Hub) Publish(m Message) {
	for k, v := range h.fields {
		m.Fields[k] = v
	}

	for _, sub := range h.matcher.Lookup(m.Topic()) {
		sub.Set(m)
	}
}

However, if Unsubscribe is called after the filtering but before the Set, the subscriber's underlying channel is closed and Set tries to write into a closed channel.

This PR adds channels and goroutines responsible for closing the channel.

r3b-fish
r3b-fish previously approved these changes Oct 19, 2020
@leandro-lugaresi
Copy link
Owner

Hey, Nice catch!
Thanks for the work on this Pull Request. I think I added this bug on PR #12 🙈

I fixed the CI process, now it should work.

hub_test.go Outdated Show resolved Hide resolved
hub_test.go Outdated Show resolved Hide resolved
hub_test.go Outdated Show resolved Hide resolved
subscriber.go Outdated Show resolved Hide resolved
@leandro-lugaresi
Copy link
Owner

The modifications are approved. Thanks for the work here!
I just fixed (I think) the GitHub actions to work on forks. Can you rebase or merge with the main branch?

@andpago
Copy link
Author

andpago commented Oct 26, 2020

I had to add an RWMutex to stop go test from getting angry at the data race. Codecov reports a decrease in code coverage, but everything in subscriber.go should be covered (at least GoLand says so). I think this happens because to get a 100% coverage you need to run the tests many times

defer s.chMu.RUnlock()

// check s.ch isn't closed (we are holding the RLock, so s.ch won't be closed until the end of this function)
select {
Copy link
Owner

Choose a reason for hiding this comment

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

You are using the closed boolean on the nonBlockingSubscriber, It's not possible to use the same here?
I think it's cleaner and makes them consistent.

Copy link
Author

Choose a reason for hiding this comment

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

I read your comment and found a bug in my implementation as well :)
Consider the following sequence of events:

  1. Set takes RLock
  2. s.closed is checked inside Set (s.closed == false)
  3. Set blocks at select for a long time
  4. Close is called, it sets s.closed to true
  5. Close tries to take Lock, but it can't, because Set is still running
  6. Deadlock

In other words, to take the lock, Close has to stop all blocking operations that have RWLock, but afaik the only way to stop a blocking operation is with a select

Copy link
Owner

Choose a reason for hiding this comment

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

About the problem of closing a channel written by several goroutines I think we have two solutions:
First, the solution described on https://www.leolara.me/blog/closing_a_go_channel_written_by_several_goroutines/: https://gist.github.com/leolara/f6fb5dfc04d64947487f16764d6b37b6
The only thing I would change is the send method not use a goroutine because this method is already been called by a goroutine and I don't want to make them async and without control.

The other solution is to change the hub and make the Unsubscribe and Publish concurrent safe using an read lock for publish (read the subscribers) and the Close, Subscribe, Unsubscribe use write lock.

WDYT?

@leandro-lugaresi
Copy link
Owner

About the codecov, you are right, We will ignore the patch.
Fix the last comment and we are ready to merge

@codecov-io
Copy link

codecov-io commented Oct 26, 2020

Codecov Report

Merging #13 into main will decrease coverage by 0.59%.
The diff coverage is 77.77%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #13      +/-   ##
==========================================
- Coverage   87.09%   86.50%   -0.60%     
==========================================
  Files           4        4              
  Lines         310      326      +16     
==========================================
+ Hits          270      282      +12     
- Misses         30       33       +3     
- Partials       10       11       +1     
Flag Coverage Δ
#unittests 86.50% <77.77%> (-0.60%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
subscriber.go 89.47% <77.77%> (-10.53%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 99dcacb...de0df9d. Read the comment docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants