asaskevich / EventBus

[Go] Lightweight eventbus with async compatibility for Go
MIT License
1.74k stars 220 forks source link

fix(event_bus): panic if multiple goroutine call publish and then wait #49

Open ken00535 opened 3 years ago

ken00535 commented 3 years ago

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()
    }
}
codecov-commenter commented 3 years ago

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%) :arrow_down:
network_bus.go 76.66% <0.00%> (-2.75%) :arrow_down:
server.go 72.13% <0.00%> (-1.79%) :arrow_down:
client.go 77.27% <0.00%> (+0.34%) :arrow_up:

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 commented 5 months ago

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