From 1d3aa703d5af272305eb634103cd4cea56cf6cd6 Mon Sep 17 00:00:00 2001 From: Nothize Date: Sat, 15 Jul 2017 20:12:27 +0800 Subject: [PATCH 1/2] Add test case for unsubscribe all and unmatched This reveals a bug that when there is more than 1 subscribers, the end result will always contain the observer to be removed. In general, where n is the length of the subscriber list when n = 1, works as expected when n > 1, subscriber list = [observer to be removed] x (n-1) --- engine_test.go | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/engine_test.go b/engine_test.go index 14e6ace..8667975 100644 --- a/engine_test.go +++ b/engine_test.go @@ -144,3 +144,28 @@ func logreply(t *testing.T, reply Reply, err error) { } t.Logf("\n") } + +func TestUnsubscribeAllAndUnmatched(t *testing.T) { + engine := NewTestEngine(t) + defer engine.ConditionalStop(t) + dummy := make(chan Reply) + rc := make(chan Reply) + engine.SubscribeAll(dummy) + engine.SubscribeAll(rc) + engine.UnsubscribeAll(rc) + for _, v := range engine.allObservers { + if v == rc { + t.Log("rc should be unsubscribed from allObservers") + t.Fail() + } + } + engine.Subscribe(dummy, UnmatchedReplyID) + engine.Subscribe(rc, UnmatchedReplyID) + engine.Unsubscribe(rc, UnmatchedReplyID) + for _, v := range engine.unObservers { + if v == rc { + t.Log("rc should be unsubscribed from unObservers") + t.Fail() + } + } +} From 914cb6fb0dacbd8aebc0f0803a1efe76804b2b10 Mon Sep 17 00:00:00 2001 From: Nothize Date: Sat, 15 Jul 2017 20:18:05 +0800 Subject: [PATCH 2/2] Fix unsubscribe all and unmatched bug that removed other observers Use a more efficient way to construct the new subscribe list while fixing the bug. --- engine.go | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/engine.go b/engine.go index 4b672a6..df6d185 100644 --- a/engine.go +++ b/engine.go @@ -436,13 +436,12 @@ func (e *Engine) Unsubscribe(o chan Reply, id int64) { return } - newUnObs := []chan<- Reply{} - for _, existing := range e.unObservers { - if existing != o { - newUnObs = append(newUnObs, o) + for i, existing := range e.unObservers { + if existing == o { + e.unObservers = append(e.unObservers[:i], e.unObservers[i+1:]...) + break } } - e.unObservers = newUnObs }) close(terminate) } @@ -460,13 +459,12 @@ func (e *Engine) UnsubscribeAll(o chan Reply) { } }() e.sendCommand(func() { - newUnObs := []chan<- Reply{} - for _, existing := range e.allObservers { - if existing != o { - newUnObs = append(newUnObs, o) + for i, existing := range e.allObservers { + if existing == o { + e.allObservers = append(e.allObservers[:i], e.allObservers[i+1:]...) + break } } - e.allObservers = newUnObs }) close(terminate) }