From 5938a711466a705c449b4995eecbc3af5243bab1 Mon Sep 17 00:00:00 2001 From: fiatjaf Date: Mon, 1 Jan 2024 10:16:07 -0300 Subject: [PATCH] replace close() with actually sending a value to .EndOfStoredEvents and .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 242af0bf76e4b23de47012244efb95ccec9e4374 by @mattn. --- eose_test.go | 52 +++++++++++++++++++++++++++++++++++++++++++++++++ subscription.go | 7 ++++--- 2 files changed, 56 insertions(+), 3 deletions(-) create mode 100644 eose_test.go diff --git a/eose_test.go b/eose_test.go new file mode 100644 index 0000000..2815a8e --- /dev/null +++ b/eose_test.go @@ -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") + } +} diff --git a/subscription.go b/subscription.go index 930bec9..ec858d3 100644 --- a/subscription.go +++ b/subscription.go @@ -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 + }() } }