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(event_bus): panic if multiple goroutine call publish and then wait #49

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ken00535
Copy link

@ken00535 ken00535 commented Jun 7, 2021

Because of wg's counter can't be a negative number, if call Wait() and then call Add(1) before Wait() complete, it will throw a panic "WaitGroup is reused before previous Wait has returned". so this PR add a mutex to fix the issue.

For more detail, see:
https://stackoverflow.com/questions/48351816/waitgroup-is-reused-before-previous-wait-unknown-reason

I write a test TestSubscribeAsyncWithMultipleGoroutine that can reproduce the issue.

func TestSubscribeAsyncWithMultipleGoroutine(t *testing.T) {
	for i := 0; i < 100; i++ {
		bus := New()
		bus.SubscribeAsync("topic", func() {
			time.Sleep(time.Millisecond)
		}, false)
		bus.Publish("topic")
		go func() {
			time.Sleep(time.Millisecond)
			bus.Publish("topic")
		}()
		bus.WaitAsync()
	}
}

because of wg's counter can't be a negative number, if call Wait() and then call Add(1) before
Wait() complete, it will throw panic "WaitGroup is reused before previous Wait has returned". so add
a mutex to prevent this issue. for more detail, see:
https://stackoverflow.com/questions/48351816/waitgroup-is-reused-before-previous-wait-unknown-reason
@codecov-commenter
Copy link

codecov-commenter commented Jun 7, 2021

Codecov Report

Merging #49 (0c59636) into master (49d4230) will decrease coverage by 1.40%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #49      +/-   ##
==========================================
- Coverage   83.26%   81.86%   -1.41%     
==========================================
  Files           4        4              
  Lines         251      215      -36     
==========================================
- Hits          209      176      -33     
+ Misses         29       26       -3     
  Partials       13       13              
Impacted Files Coverage Δ
event_bus.go 93.75% <100.00%> (-1.05%) ⬇️
network_bus.go 76.66% <0.00%> (-2.75%) ⬇️
server.go 72.13% <0.00%> (-1.79%) ⬇️
client.go 77.27% <0.00%> (+0.34%) ⬆️

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 49d4230...0c59636. Read the comment docs.

@HildaM
Copy link

HildaM commented May 21, 2024

There is a bug!
If you add lock on WaitAsync() function, it will cause TestSubcribeOnceAsync() dead lock.

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.

3 participants