Skip to content

Commit

Permalink
replace close() with actually sending a value to .EndOfStoredEvents a…
Browse files Browse the repository at this point in the history
…nd .Closed channels.

I thought `close()` would be nice because it would be cheap and not lock the goroutine while waiting for the receiver to acknowledge the thing, but turns out it introduces the serious risk of users putting <- sub.EndOfStoredEvents in the same for { select {} } statement as sub.Events, for example, and they they get into an infinite loop.

we had this same problem here inside this same library, and what is fixed in 242af0b by @mattn.
  • Loading branch information
fiatjaf committed Jan 1, 2024
1 parent 3afa6fc commit 5938a71
Show file tree
Hide file tree
Showing 2 changed files with 56 additions and 3 deletions.
52 changes: 52 additions & 0 deletions eose_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
package nostr

import (
"context"
"testing"
"time"
)

func TestEOSEMadness(t *testing.T) {
rl := mustRelayConnect(RELAY)
defer rl.Close()

sub, err := rl.Subscribe(context.Background(), Filters{
{Kinds: []int{KindTextNote}, Limit: 2},
})
if err != nil {
t.Errorf("subscription failed: %v", err)
return
}

timeout := time.After(3 * time.Second)
n := 0
e := 0

for {
select {
case event := <-sub.Events:
if event == nil {
t.Fatalf("event is nil: %v", event)
}
n++
case <-sub.EndOfStoredEvents:
e++
if e > 1 {
t.Fatalf("eose infinite loop")
}
continue
case <-rl.Context().Done():
t.Fatalf("connection closed: %v", rl.Context().Err())
case <-timeout:
goto end
}
}

end:
if e != 1 {
t.Fatalf("didn't get an eose")
}
if n < 2 {
t.Fatalf("didn't get events")
}
}
7 changes: 4 additions & 3 deletions subscription.go
Original file line number Diff line number Diff line change
Expand Up @@ -106,15 +106,16 @@ func (sub *Subscription) dispatchEose() {
if sub.eosed.CompareAndSwap(false, true) {
go func() {
sub.storedwg.Wait()
close(sub.EndOfStoredEvents)
sub.EndOfStoredEvents <- struct{}{}
}()
}
}

func (sub *Subscription) dispatchClosed(reason string) {
if sub.closed.CompareAndSwap(false, true) {
sub.ClosedReason <- reason
close(sub.ClosedReason)
go func() {
sub.ClosedReason <- reason
}()
}
}

Expand Down

0 comments on commit 5938a71

Please sign in to comment.