From 66ea0a2b1f3ccb7a1956230ff1d89d8e6452d2dc Mon Sep 17 00:00:00 2001 From: Felix Lange Date: Thu, 10 May 2018 12:26:36 +0200 Subject: [PATCH 1/4] event: document select case slice use and add edge case test (#16680) Feed keeps active subscription channels in a slice called 'f.sendCases'. The Send method tracks the active cases in a local variable 'cases' whose value is f.sendCases initially. 'cases' shrinks to a shorter prefix of f.sendCases every time a send succeeds, moving the successful case out of range of the active case list. This can be confusing because the two slices share a backing array. Add more comments to document what is going on. Also add a test for removing a case that is in 'f.sentCases' but not 'cases'. --- event/feed.go | 5 ++++- event/feed_test.go | 39 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 43 insertions(+), 1 deletion(-) diff --git a/event/feed.go b/event/feed.go index 78fa3d98d8eb..f578f00c10ff 100644 --- a/event/feed.go +++ b/event/feed.go @@ -148,7 +148,9 @@ func (f *Feed) Send(value interface{}) (nsent int) { f.sendCases[i].Send = rvalue } - // Send until all channels except removeSub have been chosen. + // Send until all channels except removeSub have been chosen. 'cases' tracks a prefix + // of sendCases. When a send succeeds, the corresponding case moves to the end of + // 'cases' and it shrinks by one element. cases := f.sendCases for { // Fast path: try sending without blocking before adding to the select set. @@ -170,6 +172,7 @@ func (f *Feed) Send(value interface{}) (nsent int) { index := f.sendCases.find(recv.Interface()) f.sendCases = f.sendCases.delete(index) if index >= 0 && index < len(cases) { + // Shrink 'cases' too because the removed case was still active. cases = f.sendCases[:len(cases)-1] } } else { diff --git a/event/feed_test.go b/event/feed_test.go index 6c4c91b5b495..8713e79bd181 100644 --- a/event/feed_test.go +++ b/event/feed_test.go @@ -236,6 +236,45 @@ func TestFeedUnsubscribeBlockedPost(t *testing.T) { wg.Wait() } +// Checks that unsubscribing a channel during Send works even if that +// channel has already been sent on. +func TestFeedUnsubscribeSentChan(t *testing.T) { + var ( + feed Feed + ch1 = make(chan int) + ch2 = make(chan int) + sub1 = feed.Subscribe(ch1) + sub2 = feed.Subscribe(ch2) + wg sync.WaitGroup + ) + defer sub2.Unsubscribe() + + wg.Add(1) + go func() { + feed.Send(0) + wg.Done() + }() + + // Wait for the value on ch1. + <-ch1 + // Unsubscribe ch1, removing it from the send cases. + sub1.Unsubscribe() + + // Receive ch2, finishing Send. + <-ch2 + wg.Wait() + + // Send again. This should send to ch2 only, so the wait group will unblock + // as soon as a value is received on ch2. + wg.Add(1) + go func() { + feed.Send(0) + wg.Done() + }() + <-ch2 + wg.Wait() +} + func TestFeedUnsubscribeFromInbox(t *testing.T) { var ( feed Feed From d7c71ca97925f3236b74370fb380447d1cc4071e Mon Sep 17 00:00:00 2001 From: Daniel Liu Date: Tue, 27 Aug 2024 06:45:22 +0800 Subject: [PATCH 2/4] event: remove unused field closed (#20324) --- event/feed.go | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/event/feed.go b/event/feed.go index f578f00c10ff..02f3ca68759f 100644 --- a/event/feed.go +++ b/event/feed.go @@ -39,10 +39,9 @@ type Feed struct { sendCases caseList // the active set of select cases used by Send // The inbox holds newly subscribed channels until they are added to sendCases. - mu sync.Mutex - inbox caseList - etype reflect.Type - closed bool + mu sync.Mutex + inbox caseList + etype reflect.Type } // This is the index of the first actual subscription channel in sendCases. From 6012aa682fefeacd298f82c9e8e771069226e183 Mon Sep 17 00:00:00 2001 From: Daniel Liu Date: Tue, 27 Aug 2024 06:25:54 +0800 Subject: [PATCH 3/4] event: add missing unlock before panic (#20653) --- event/feed.go | 1 + 1 file changed, 1 insertion(+) diff --git a/event/feed.go b/event/feed.go index 02f3ca68759f..33dafe58867e 100644 --- a/event/feed.go +++ b/event/feed.go @@ -138,6 +138,7 @@ func (f *Feed) Send(value interface{}) (nsent int) { if !f.typecheck(rvalue.Type()) { f.sendLock <- struct{}{} + f.mu.Unlock() panic(feedTypeError{op: "Send", got: rvalue.Type(), want: f.etype}) } f.mu.Unlock() From 23b7743a8583ab16f347292f0ddea721291aac31 Mon Sep 17 00:00:00 2001 From: Daniel Liu Date: Tue, 27 Aug 2024 06:59:54 +0800 Subject: [PATCH 4/4] event: include Feed type fixation logic in f.init (#27249) --- event/feed.go | 34 ++++++++++++---------------------- 1 file changed, 12 insertions(+), 22 deletions(-) diff --git a/event/feed.go b/event/feed.go index 33dafe58867e..d94bd820f0c5 100644 --- a/event/feed.go +++ b/event/feed.go @@ -57,7 +57,8 @@ func (e feedTypeError) Error() string { return "event: wrong type in " + e.op + " got " + e.got.String() + ", want " + e.want.String() } -func (f *Feed) init() { +func (f *Feed) init(etype reflect.Type) { + f.etype = etype f.removeSub = make(chan interface{}) f.sendLock = make(chan struct{}, 1) f.sendLock <- struct{}{} @@ -70,8 +71,6 @@ func (f *Feed) init() { // The channel should have ample buffer space to avoid blocking other subscribers. // Slow subscribers are not dropped. func (f *Feed) Subscribe(channel interface{}) Subscription { - f.once.Do(f.init) - chanval := reflect.ValueOf(channel) chantyp := chanval.Type() if chantyp.Kind() != reflect.Chan || chantyp.ChanDir()&reflect.SendDir == 0 { @@ -79,11 +78,13 @@ func (f *Feed) Subscribe(channel interface{}) Subscription { } sub := &feedSub{feed: f, channel: chanval, err: make(chan error, 1)} - f.mu.Lock() - defer f.mu.Unlock() - if !f.typecheck(chantyp.Elem()) { + f.once.Do(func() { f.init(chantyp.Elem()) }) + if f.etype != chantyp.Elem() { panic(feedTypeError{op: "Subscribe", got: chantyp, want: reflect.ChanOf(reflect.SendDir, f.etype)}) } + + f.mu.Lock() + defer f.mu.Unlock() // Add the select case to the inbox. // The next Send will add it to f.sendCases. cas := reflect.SelectCase{Dir: reflect.SelectSend, Chan: chanval} @@ -91,15 +92,6 @@ func (f *Feed) Subscribe(channel interface{}) Subscription { return sub } -// note: callers must hold f.mu -func (f *Feed) typecheck(typ reflect.Type) bool { - if f.etype == nil { - f.etype = typ - return true - } - return f.etype == typ -} - func (f *Feed) remove(sub *feedSub) { // Delete from inbox first, which covers channels // that have not been added to f.sendCases yet. @@ -128,19 +120,17 @@ func (f *Feed) remove(sub *feedSub) { func (f *Feed) Send(value interface{}) (nsent int) { rvalue := reflect.ValueOf(value) - f.once.Do(f.init) + f.once.Do(func() { f.init(rvalue.Type()) }) + if f.etype != rvalue.Type() { + panic(feedTypeError{op: "Send", got: rvalue.Type(), want: f.etype}) + } + <-f.sendLock // Add new cases from the inbox after taking the send lock. f.mu.Lock() f.sendCases = append(f.sendCases, f.inbox...) f.inbox = nil - - if !f.typecheck(rvalue.Type()) { - f.sendLock <- struct{}{} - f.mu.Unlock() - panic(feedTypeError{op: "Send", got: rvalue.Type(), want: f.etype}) - } f.mu.Unlock() // Set the sent value on all channels.