feat(batch): configurable context-cancellation mode (WithCancelMode)#76
feat(batch): configurable context-cancellation mode (WithCancelMode)#76MasterOfBinary wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a configurable CancelMode for the batch processing engine, allowing users to choose between draining already-read items (CancelDrain, the default) or stopping promptly upon context cancellation (CancelStop). The feedback focuses on adding input validation for the CancelMode parameter to prevent invalid integer values, and refactoring the tests to use a single, robust releasableSource to eliminate goroutine leaks caused by the uncooperative test source blocking indefinitely.
| func (b *Batch[T]) WithCancelMode(m CancelMode) *Batch[T] { | ||
| b.mu.Lock() | ||
| defer b.mu.Unlock() | ||
|
|
||
| if b.running { | ||
| panic("batch: WithCancelMode cannot be called after Go() has started") | ||
| } | ||
|
|
||
| b.cancelMode = m | ||
| return b | ||
| } |
There was a problem hiding this comment.
To prevent bugs and ensure defensive programming, it is highly recommended to validate the CancelMode input in WithCancelMode. Since CancelMode is an integer type, an invalid value (e.g., 42) could be passed, which would silently fall back to CancelDrain behavior and make debugging difficult.
func (b *Batch[T]) WithCancelMode(m CancelMode) *Batch[T] {
b.mu.Lock()
defer b.mu.Unlock()
if b.running {
panic("batch: WithCancelMode cannot be called after Go() has started")
}
if m != CancelDrain && m != CancelStop {
panic(fmt.Sprintf("batch: invalid cancel mode %d", m))
}
b.cancelMode = m
return b
}| // uncooperativeSource emits a fixed number of items, then blocks forever | ||
| // without ever closing its channels and WITHOUT honoring ctx.Done(). This is | ||
| // the worst-case Source for the engine: in CancelDrain mode the reader has no | ||
| // way to make progress after cancellation because it is waiting on a Source | ||
| // that never stops. It is the precise scenario that distinguishes CancelStop | ||
| // from CancelDrain. | ||
| // | ||
| // release (returned to the caller) unblocks the producer and closes the | ||
| // channels so a test that relies on drain semantics can still clean up without | ||
| // leaking the goroutine. | ||
| type uncooperativeSource struct { | ||
| emit int // how many items to emit before blocking | ||
| delivered *uint32 // optional: incremented after each successful send into the pipeline | ||
| started chan struct{} // closed once the producer goroutine is running | ||
| startOnce sync.Once | ||
| } | ||
|
|
||
| func newUncooperativeSource(emit int, delivered *uint32) *uncooperativeSource { | ||
| return &uncooperativeSource{ | ||
| emit: emit, | ||
| delivered: delivered, | ||
| started: make(chan struct{}), | ||
| } | ||
| } | ||
|
|
||
| func (s *uncooperativeSource) Read(ctx context.Context) (<-chan any, <-chan error) { | ||
| out := make(chan any) | ||
| errs := make(chan error) | ||
| block := make(chan struct{}) // never closed; the producer parks here forever | ||
|
|
||
| go func() { | ||
| // Intentionally do NOT close(out)/close(errs) and do NOT select on | ||
| // ctx.Done(): this Source ignores cancellation entirely. | ||
| s.startOnce.Do(func() { close(s.started) }) | ||
| for i := 0; i < s.emit; i++ { | ||
| out <- i // blocks until the reader takes it (unbuffered) | ||
| if s.delivered != nil { | ||
| atomic.AddUint32(s.delivered, 1) | ||
| } | ||
| } | ||
| <-block // park forever | ||
| }() | ||
|
|
||
| return out, errs | ||
| } |
There was a problem hiding this comment.
The uncooperativeSource intentionally leaks a goroutine because it blocks forever on <-block or out <- i after the reader stops reading. Leaking goroutines in tests can lead to resource exhaustion, flaky tests, or test failures (especially if leak detectors are used).
We can completely eliminate uncooperativeSource and these goroutine leaks by using releasableSource everywhere, provided we update releasableSource to support interruption during channel sends (by selecting on s.release).
Let's define a single, robust releasableSource here:
// releasableSource emits a fixed number of items, then blocks until Release() is called.
// It does not honor ctx.Done(), simulating an uncooperative source, but can be
// explicitly released to prevent goroutine leaks in tests.
type releasableSource struct {
emit int
delivered *uint32
started chan struct{}
release chan struct{}
startOnce sync.Once
}
func newReleasableSource(emit int, delivered *uint32) *releasableSource {
return &releasableSource{
emit: emit,
delivered: delivered,
started: make(chan struct{}),
release: make(chan struct{}),
}
}
func (s *releasableSource) Release() {
s.startOnce.Do(func() { close(s.started) })
close(s.release)
}
func (s *releasableSource) Read(ctx context.Context) (<-chan any, <-chan error) {
out := make(chan any)
errs := make(chan error)
go func() {
defer close(out)
defer close(errs)
s.startOnce.Do(func() { close(s.started) })
for i := 0; i < s.emit; i++ {
select {
case out <- i:
case <-s.release:
return
}
if s.delivered != nil {
atomic.AddUint32(s.delivered, 1)
}
}
<-s.release
}()
return out, errs
}| func TestCancelStop_StopsOnUncooperativeSource(t *testing.T) { | ||
| ctx, cancel := context.WithCancel(context.Background()) | ||
| defer cancel() | ||
|
|
||
| src := newUncooperativeSource(2, nil) | ||
|
|
||
| b := New[any](nil).WithCancelMode(CancelStop) | ||
| IgnoreErrors(b.Go(ctx, src, &noopProc{})) | ||
|
|
||
| // Make sure the source is actually running before we cancel, so we are | ||
| // exercising the cancel-while-reading path rather than a pre-cancel race. | ||
| <-src.started | ||
| cancel() | ||
|
|
||
| select { | ||
| case <-b.Done(): | ||
| // Pipeline stopped promptly on cancel despite the Source never closing. | ||
| case <-time.After(2 * time.Second): | ||
| t.Fatal("CancelStop: Done() did not close within 2s on an uncooperative source") | ||
| } | ||
| } |
There was a problem hiding this comment.
Update TestCancelStop_StopsOnUncooperativeSource to use releasableSource and call Release() to prevent goroutine leaks.
func TestCancelStop_StopsOnUncooperativeSource(t *testing.T) {
ctx, cancel := context.WithCancel(context.Background())
defer cancel()
src := newReleasableSource(2, nil)
defer src.Release()
b := New[any](nil).WithCancelMode(CancelStop)
IgnoreErrors(b.Go(ctx, src, &noopProc{}))
// Make sure the source is actually running before we cancel, so we are
// exercising the cancel-while-reading path rather than a pre-cancel race.
<-src.started
cancel()
select {
case <-b.Done():
// Pipeline stopped promptly on cancel despite the Source never closing.
case <-time.After(2 * time.Second):
t.Fatal("CancelStop: Done() did not close within 2s on an uncooperative source")
}
}| func TestCancelDrain_DefaultRelaysOnSource(t *testing.T) { | ||
| ctx, cancel := context.WithCancel(context.Background()) | ||
| defer cancel() | ||
|
|
||
| src := newUncooperativeSource(2, nil) | ||
|
|
||
| // No WithCancelMode call => default CancelDrain. | ||
| b := New[any](nil) | ||
| IgnoreErrors(b.Go(ctx, src, &noopProc{})) | ||
|
|
||
| <-src.started | ||
| cancel() | ||
|
|
||
| // In drain mode the engine must NOT complete on its own while the Source | ||
| // keeps its channels open. | ||
| select { | ||
| case <-b.Done(): | ||
| t.Fatal("CancelDrain: Done() closed on its own; default must rely on the Source to stop") | ||
| case <-time.After(200 * time.Millisecond): | ||
| // Expected: still waiting on the Source. | ||
| } | ||
|
|
||
| // Cleanup: there is no way to release this particular Source's producer | ||
| // from the test (block is internal), so leave the deferred cancel in place; | ||
| // the parked producer goroutine is harmless for the remainder of the test | ||
| // binary. To keep the engine itself from hanging the suite, we do not block | ||
| // on Done() here. | ||
| _ = ctx | ||
| } |
There was a problem hiding this comment.
Update TestCancelDrain_DefaultRelaysOnSource to use releasableSource and call Release() to prevent goroutine leaks and clean up the unused _ = ctx variable.
func TestCancelDrain_DefaultRelaysOnSource(t *testing.T) {
ctx, cancel := context.WithCancel(context.Background())
defer cancel()
src := newReleasableSource(2, nil)
defer src.Release()
// No WithCancelMode call => default CancelDrain.
b := New[any](nil)
IgnoreErrors(b.Go(ctx, src, &noopProc{}))
<-src.started
cancel()
// In drain mode the engine must NOT complete on its own while the Source
// keeps its channels open.
select {
case <-b.Done():
t.Fatal("CancelDrain: Done() closed on its own; default must rely on the Source to stop")
case <-time.After(200 * time.Millisecond):
// Expected: still waiting on the Source.
}
}| type releasableSource struct { | ||
| emit int | ||
| delivered *uint32 | ||
| started chan struct{} | ||
| release chan struct{} | ||
| startOnce sync.Once | ||
| } | ||
|
|
||
| func newReleasableSource(emit int, delivered *uint32) *releasableSource { | ||
| return &releasableSource{ | ||
| emit: emit, | ||
| delivered: delivered, | ||
| started: make(chan struct{}), | ||
| release: make(chan struct{}), | ||
| } | ||
| } | ||
|
|
||
| // Release unblocks the producer and lets it close its channels. | ||
| func (s *releasableSource) Release() { close(s.release) } | ||
|
|
||
| func (s *releasableSource) Read(ctx context.Context) (<-chan any, <-chan error) { | ||
| out := make(chan any) | ||
| errs := make(chan error) | ||
|
|
||
| go func() { | ||
| defer close(out) | ||
| defer close(errs) | ||
| s.startOnce.Do(func() { close(s.started) }) | ||
| for i := 0; i < s.emit; i++ { | ||
| out <- i | ||
| if s.delivered != nil { | ||
| atomic.AddUint32(s.delivered, 1) | ||
| } | ||
| } | ||
| <-s.release // ignore ctx; only Release() (or process exit) frees us | ||
| }() | ||
|
|
||
| return out, errs | ||
| } |
| func TestCancelStop_ProcessesBufferedItems(t *testing.T) { | ||
| ctx, cancel := context.WithCancel(context.Background()) | ||
| defer cancel() | ||
|
|
||
| const emit = 5 | ||
| var delivered uint32 | ||
| var processed uint32 | ||
|
|
||
| src := newUncooperativeSource(emit, &delivered) | ||
|
|
||
| proc := &countingProc{counter: &processed} | ||
|
|
||
| // Large item buffer so all delivered items sit buffered ahead of the | ||
| // processor, and MinItems=1 so each item forms its own batch and is | ||
| // processed promptly. | ||
| b := New[any](NewConstantConfig(&ConfigValues{MinItems: 1})). | ||
| WithCancelMode(CancelStop). | ||
| WithBufferConfig(BufferConfig{ItemBufferSize: 100, ErrorBufferSize: 100}) | ||
| IgnoreErrors(b.Go(ctx, src, proc)) | ||
|
|
||
| // Wait until the Source has delivered all `emit` items into the pipeline. | ||
| deadline := time.After(2 * time.Second) | ||
| for atomic.LoadUint32(&delivered) < emit { | ||
| select { | ||
| case <-deadline: | ||
| t.Fatalf("source only delivered %d/%d items before timeout", | ||
| atomic.LoadUint32(&delivered), emit) | ||
| default: | ||
| time.Sleep(time.Millisecond) | ||
| } | ||
| } | ||
|
|
||
| cancel() | ||
|
|
||
| select { | ||
| case <-b.Done(): | ||
| case <-time.After(2 * time.Second): | ||
| t.Fatal("CancelStop: Done() did not close within 2s") | ||
| } | ||
|
|
||
| if got := atomic.LoadUint32(&processed); got != emit { | ||
| t.Fatalf("CancelStop dropped buffered work: processed %d, want %d", got, emit) | ||
| } | ||
| } |
There was a problem hiding this comment.
Update TestCancelStop_ProcessesBufferedItems to use releasableSource and call Release() to prevent goroutine leaks.
func TestCancelStop_ProcessesBufferedItems(t *testing.T) {
ctx, cancel := context.WithCancel(context.Background())
defer cancel()
const emit = 5
var delivered uint32
var processed uint32
src := newReleasableSource(emit, &delivered)
defer src.Release()
proc := &countingProc{counter: &processed}
// Large item buffer so all delivered items sit buffered ahead of the
// processor, and MinItems=1 so each item forms its own batch and is
// processed promptly.
b := New[any](NewConstantConfig(&ConfigValues{MinItems: 1})).
WithCancelMode(CancelStop).
WithBufferConfig(BufferConfig{ItemBufferSize: 100, ErrorBufferSize: 100})
IgnoreErrors(b.Go(ctx, src, proc))
// Wait until the Source has delivered all `emit` items into the pipeline.
deadline := time.After(2 * time.Second)
for atomic.LoadUint32(&delivered) < emit {
select {
case <-deadline:
t.Fatalf("source only delivered %d/%d items before timeout",
atomic.LoadUint32(&delivered), emit)
default:
time.Sleep(time.Millisecond)
}
}
cancel()
select {
case <-b.Done():
case <-time.After(2 * time.Second):
t.Fatal("CancelStop: Done() did not close within 2s")
}
if got := atomic.LoadUint32(&processed); got != emit {
t.Fatalf("CancelStop dropped buffered work: processed %d, want %d", got, emit)
}
}1fd4f94 to
f689df1
Compare
80547a8 to
4b16ec0
Compare
f689df1 to
027b25a
Compare
…ncelStop) Add a CancelMode knob controlling how a Batch reacts to context cancellation, plus a WithCancelMode builder that mirrors WithBufferConfig (guards b.running and panics if called after Go). CancelDrain is the zero value and preserves today's behavior exactly: the reader keeps waiting on the Source and relies on it to close its channels. CancelStop wires a stop case into doReader's select via the nil-channel trick (stopCh is nil in drain mode, so it never fires), promptly closing b.items on cancellation while leaving already-buffered items to be processed. The context-aware internal error sends are unchanged in both modes. Godoc on the constants, the builder, and Go's cancellation block is updated. No exported signature changes; the feature is purely additive and the zero value equals the prior default. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add cancelmode_test.go using an uncooperative Source that emits a few items then blocks forever, ignoring ctx, to distinguish the two modes: - CancelStop drives the pipeline to completion on cancel (vs hanging). - CancelStop still processes items buffered before cancel (no dropped work). - Default/CancelDrain does NOT self-stop on the uncooperative Source, and a releasable variant confirms drain completes once the Source closes. - WithCancelMode panics if called after Go. - The zero-value mode is CancelDrain and a cooperative Source completes in all modes. All concurrency assertions are bounded by 2s timeouts so failures surface fast instead of hanging. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Update WithCancelMode's guard to b.used (single-use renamed running→used) and fix the cancel-mode tests for Go's (<-chan error, error) signature. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
4b16ec0 to
8add152
Compare
Summary
Adds a configurable context-cancellation policy so callers choose how a
Batchreacts to cancellation — settling the recurring drain-vs-stop review debate with an explicit knob instead of one hardcoded behavior.API (backwards-compatible — zero value == today's behavior)
CancelDrain(default): on cancel, keep processing items already read from the Source, relying on the Source to stop and close its channels. Nothing already read into the pipeline is dropped. This is the existing behavior, so current users see no change.CancelStop: on cancel, stop reading promptly instead of waiting for the Source. Items already buffered are still processed; items not yet read from the Source may be dropped.Implementation
cancelModefield +WithCancelModebuilder, mirroringWithBufferConfig(settable only beforeGo(); panics if called after).CancelStopenables acase <-ctx.Done()arm indoReader's select via the nil-channel trick — the case is inert (nil channel) inCancelDrain, so the default path is byte-for-byte unchanged.Testing (TDD)
CancelStopstops on an uncooperative (ctx-ignoring) source;CancelDraindoes not (relies on the Source) — both RED tests hung for 2 s before wiring, then passed.CancelStopstill processes already-buffered items (drops nothing already read).WithCancelModepanics afterGo(); default mode isCancelDrain.go test -race ./...green; existing hardening/wedge tests unaffected.Backwards compatibility
Purely additive.
CancelDrainis the zero value, so aBatchwith noWithCancelModecall behaves identically to before.🤖 Generated with Claude Code