add Notify(): a selectable channel for consumers that can't block in Read - #32
Conversation
…ources don't have to build a wrapper
smallnest
left a comment
There was a problem hiding this comment.
Review: Notify()
Solid work — I traced every path that advances r.w and actively tried to falsify the coalescing design; I could not find a correctness defect. The items below are documentation and test coverage, not code defects.
Verified locally
gofmt -l .clean,go vet ./...clean,go test -race -count=1 ./...green (~12s);notify_test.gorun x20 under-racewith no flakes.- Signal coverage matches the rationale in the description. Funnels
write(ring_buffer.go:768) andwriteByte(:853), plusReadFrom(:575),setErr(:194) andReset(:1031). Probes confirmWrite,WriteString,WriteByte,TryWrite,TryWriteByte, overwrite mode,ReadFrom,Resetand close/close-with-error all fire. - The cap-1 buffered channel really does make peek -> select -> consume lossless for a single consumer: a write landing between "peek empty" and "select" leaves a pending token. Stress test (300 rounds x 500 writes) clean.
- Non-blocking send with
writeMu+muheld is safe; no deadlock is possible.
1. One signal wakes one waiter — please document it (or broadcast)
chan struct{} is point-to-point: with N goroutines selecting on Notify(), a single write wakes exactly one of them (probe: 1 of 3). Read and the conds are broadcast-based, so the buffer's own model is multi-reader while Notify quietly is not. Please state in the Notify doc that it is single-waiter / not a broadcast, or switch to close-and-replace if multi-waiter is intended.
2. A wakeup that is received but not acted on is spent
The token is consumed by the receive, so a consumer that drains a signal without re-inspecting (or that "clears stale signals" in a loop) can park with data buffered — I reproduced a stall at 187 bytes buffered. The doc says a wakeup means "look, not there is data"; it should also say "never discard a wakeup without looking, and re-inspect before parking."
3. ReadFrom needs a signal test
The description calls ReadFrom out as the exception that bypasses the funnels, but nothing asserts Notify fires for it (nor for overwrite / Reset / WriteByte / Try*). The behavior is correct; a test would lock in the coverage argument. Suggested:
func TestNotifyOnReadFrom(t *testing.T) {
rb := New(64).SetBlocking(true)
pr, pw := io.Pipe()
go func() { _, _ = rb.ReadFrom(pr) }()
time.Sleep(50 * time.Millisecond) // let ReadFrom park in rd.Read
go func() { _, _ = pw.Write([]byte("hi")) }()
if !waitNotify(rb, 2*time.Second) {
t.Fatal("no signal after ReadFrom committed")
}
if rb.Length() != 2 {
t.Fatalf("Length = %d, want 2", rb.Length())
}
_ = pw.Close()
}4. Nits
drain(t, rb)innotify_test.godoes not drain anything; it registers a cleanup that closes the buffer. Rename (e.g.closeOnCleanup) so the name and comment match what it does.- Zero value:
var rb RingBufferyields a nilNotify()channel (receives block forever) and a no-opsignalNotify. No panic, and the zero value is already unusable, so this is just a sentence or lazy init if you care. README.mddocuments blocking/non-blocking behavior but says nothing aboutNotify. A short section would help, since the Peek-without-consuming pattern is the whole point.
Praise
- Signalling outside the
if r.blockguard and allocatingnotifyin both constructors is the right call; non-blocking consumers are the likelier users. - Signalling from the
write/writeBytefunnels rather than beside eachBroadcastis well reasoned and future-proofs new callers. - Backward compatible: one field and one channel per buffer, no change to existing methods.
Note: no CI checks are configured on feature/notify, so the above is from local runs. Happy to approve once 1–3 are addressed.
|
Follow-up for the review nits on Feel free to take it, ignore it, or fold the changes in differently. |
- ring_buffer.go: Notify's doc now states it is not a broadcast (one signal wakes one waiter, single waiting consumer per buffer) and that a received wakeup must not be discarded without re-inspecting. - notify_test.go: add TestNotifyOnReadFrom, since ReadFrom bypasses the write funnels and signals at its own commit point. Rename drain to closeOnCleanup to match what the helper does. - README.md: short section on waiting without blocking via Notify.
|
I merged and cherry picked |
Problem
A
RingBuffercannot be waited on as part of aselect.A consumer that has only the buffer to worry about blocks in
Readand isserved well by the existing APIs. But a consumer waiting on several things at
once — new data, plus a shutdown signal, a deadline, or events from elsewhere —
needs every wakeup source to be selectable, and a blocking
Readis not one.Today that means wrapping the buffer in a goroutine that blocks in
Readandforwards to a channel. That works, but it forces the consumer to consume bytes
in order to learn that bytes exist, which is precisely wrong for anything built
on
Peek: code that inspects buffered data without taking it, because taking itis what confirmation or acknowledgement is for.
Peekis non-blocking bydesign, so such a consumer has nothing to wait on at all and is left polling.
API
Signaled when data is written, and when the buffer is closed or reset.
Contract
Deliberately weak, and the weakness is the point:
how much or how often. A pending signal already conveys everything a later one
would, so dropping the later one loses nothing — and it means a slow consumer
needs no buffering policy and can never apply backpressure to a writer.
buffer. Spurious signals are permitted, which is what keeps the signalling
sites free to be conservative.
consumer is entitled to this too, and is in fact likelier to need it.
Where signals are raised, and why not beside every Broadcast
writeCond.Broadcastlives in the exported entry points.signalNotifyisraised from the internal
write()andwriteByte()funnels instead, for tworeasons:
writeBytehas four callers and only two broadcast; the rest arecovered transitively, which takes tracing to confirm. Signalling at the funnel
means a newly added caller cannot be missed.
if r.blockguards, because theconds are nil otherwise. Mirroring that placement would silence
Notifyfor anon-blocking buffer. For the same reason
setErrsignals outside its guard: aconsumer still has to learn the buffer closed.
ReadFromis the exception that proves the rule — it writes directly intor.bufand bypasses the funnels, so it signals at its own commit point, rightafter
writeCond.Broadcast().Interaction with #29
Rebased onto the new locking. Two things were checked rather than assumed:
r.wnow signals:ReadFrom,write(x2),writeByte,Reset(x2). A missed one would leave consumers asleep throughwrites that really happened, which no test would necessarily catch.
signalNotifyruns withwriteMuandr.muheld inReadFromandwrite.Safe because the send is non-blocking — a blocking send there would deadlock
against a consumer needing
r.muto drain.Tests
notify_test.go:TestNotifyOnWrite— the basic signalTestNotifyWorksInNonBlockingMode— the guard placement aboveTestNotifyCoalesces— many writes, one pending signalTestNotifyOnClose/TestNotifyOnCloseWithError— closure is observableTestNotifyDoesNotConsume— a wakeup leaves the read pointer aloneTestPeekOnlyNeverSeesEOF— the Peek-without-consuming case that motivated thisTestNotifyDrivesPeekConsumeLoop— end to end: select on Notify, peek, consumeNote
Three struct-field comments realign in the diff:
chan struct{}is wider thanthe existing types, so gofmt re-columns
readCond,writeCondandgeneration. No change to those fields.gofmt -l .empty,go vet ./...clean,go test -race -count=1 ./...greenagainst current master (go1.26.3).