feat(batch)!: single-use Batch + Go returns (<-chan error, error); inline item IDs#64
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #64 +/- ##
==========================================
- Coverage 96.79% 96.73% -0.06%
==========================================
Files 12 12
Lines 374 368 -6
==========================================
- Hits 362 356 -6
Misses 9 9
Partials 3 3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Code Review
This pull request addresses data races and improves the reliability of reusing Batch instances. Key changes include passing channels to the ID generator by value to prevent field access races, introducing an idDone channel to ensure background goroutines have fully exited before shutdown completes, and replacing time.After with time.NewTimer to prevent timer leaks. Documentation and examples for CollectErrors were also updated to clarify its blocking behavior. Feedback was provided regarding a remaining race condition where the running flag is cleared after the Done() channel is closed, which could lead to panics if the batch is reused immediately after waiting on Done().
99a7410 to
27ee6f5
Compare
27ee6f5 to
a042228
Compare
|
Removed the For the record: with the start-error contract in this PR, a nil source is reported by
|
…move IDBufferSize) Replace the dedicated ID-generator goroutine and its buffered channel with an inline counter on Batch, reset on each Go() so a reused Batch numbers from zero. Only Go (reset, before goroutines start) and the single doReader goroutine touch it, so a plain counter is race-free — no atomic or lock, and no 64-bit-alignment concern on 32-bit platforms. Removes BufferConfig.IDBufferSize and the DefaultIDBufferSize constant (BREAKING) and eliminates a pre-existing Batch-reuse data race on ID generation. Finalize shutdown under b.mu — close done and clear running before closing errs — so once CollectErrors returns or Done() fires the Batch is fully torn down and reusable without tripping the "Concurrent calls" guard. Fix a timer leak in waitForItems (time.NewTimer + defer Stop() + Reset() instead of time.After), and clarify that CollectErrors blocks until the error channel closes. Tests: reuse loops including a test pinning the per-run ID reset to 0..n-1, plus a throughput benchmark. The inline counter benchmarks ~40% faster than the previous channel-based generator. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Replaces the reusable-Batch design with single-use semantics, which removes a whole class of cross-run concurrency hazards (the shared ID-counter race and the Done()/Go() reuse race surfaced in review). - Go now returns (<-chan error, error). Start failures surface via the returned error using errors.Is-able sentinels (ErrBatchUsed, ErrNilSource) instead of panicking (second/concurrent call) or smuggling the error onto the pipeline channel (nil source). On a start error the returned channel is non-nil and already closed, so ranging over it is always safe. - A Batch runs once; a second Go() returns ErrBatchUsed. Create a new Batch with New to run again. - The item-ID counter is now a doReader-local variable, so it needs no atomic or lock by construction. - Shutdown drops the mutex around closing done/errs (nothing reassigns them without reuse). - RunBatchAndWait/ExecuteBatches surface start errors; all call sites, examples, README, and docs updated. reuse_test.go removed; single_use_test.go added. BREAKING CHANGE: Batch.Go returns (<-chan error, error) and Batch is single-use. Update call sites to `errs, err := b.Go(...)` and create a new Batch per run. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… default ExecuteBatches skipped a BatchConfig whose Source was nil before ever calling Go, so a missing source produced no work and no error and callers could advance as if the batch had completed. Drop the nil-source check from the skip guard so cfg.B.Go reports ErrNilSource, which is then collected. Nil *BatchConfig / nil Batch entries remain a tolerated silent skip (unchanged, separately tested contract). Docs: note in the README that a nil Config (or the zero-value Batch) uses the default configuration, and add the missing "log" import to the Basic Usage example. Adds TestExecuteBatches_NilSourceSurfacesError. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Capture and assert err == nil after Batch.Go in tests instead of discarding it, so an unexpected start error fails loudly rather than surfacing as a confusing downstream symptom. Two Go calls inside spawned goroutines are left discarding the error (t.Fatal is illegal off the test goroutine, and a downstream assertion already catches a start error there). Also drop a review-only comment in error_handling_test.go that explained where nil-source coverage moved; that context belongs on the PR, not in the code. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The single-use refactor made Go return (<-chan error, error); this call site still dropped the error, which golangci-lint's errcheck flags. Capture and assert the start error and drain the channel, matching the other call sites. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
3b59001 to
d49ace6
Compare
Rebased fix/doc-corrections onto master (ba2a756), which merged the single-use Batch API (#64) and the golangci-lint v2.12.2 install line (#78) into the same doc files this PR edits. The rebase auto-merged without textual conflicts, but doc.go required a genuine three-way reconciliation: master rewrote the package-doc snippet to the new API (errs, err := b.Go(...); if err != nil { log.Fatal(err) }; batch.IgnoreErrors(errs)) while this PR independently removed the misleading "// Output:" marker from that same block. Both edits were combined off the common ancestor, so the result keeps the new API and drops the marker — no manual conflict markers were ever produced. Reconciliation outcome: - Kept master's content: single-use semantics, Go's (<-chan error, error) signature, ErrBatchUsed/ErrNilSource, nil-Config default note, the removal of IDBufferSize, and the golangci-lint install command. - Re-applied this PR's still-valid corrections on top: Nil processor now documented as sleeping for a Duration (CLAUDE.md/AGENTS.md/README.md), errors.As pointer-target guidance (CLAUDE.md/AGENTS.md, verified against the &SourceError{}/&ProcessorError{} sends in batch/batch.go), File Structure refresh, and the doc.go / source/doc.go "// Output:" cleanups. - Dropped nothing as redundant: master and this PR touched disjoint concerns, so every correction this PR intended still applies. Verified post-rebase: go build ./..., go vet ./..., golangci-lint v2.12.2 run --timeout=3m (0 issues), go test ./... (all packages pass, including the runnable root Example), gofmt -l . empty. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Rebase the deterministic-example work onto the single-use Batch API (#64). Batch.Go now returns (<-chan error, error) and a Batch is single-use, so the examples that call Go directly use `errs, err := b.Go(...)` and check the start error. Examples remain deterministic under both normal scheduling and GOMAXPROCS=1: single-batch configs (MinItems==MaxItems) collapse the pipeline into one goroutine for the chained/per-batch-print examples, and Example_dynamicConfig counts items under a mutex and prints an invariant summary after <-b.Done() instead of racing per-batch output. Example_customConfig and Example_simpleProcessor-style helpers (RunBatchAndWait) were already API-compatible and are left as master's where appropriate. Verified: gofmt -l (clean), go vet, go build, golangci-lint (0 issues), go test -race, and go test -run Example -count=20 both normally and with GOMAXPROCS=1. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* test(batch): reconcile deterministic examples with single-use API Rebase the deterministic-example work onto the single-use Batch API (#64). Batch.Go now returns (<-chan error, error) and a Batch is single-use, so the examples that call Go directly use `errs, err := b.Go(...)` and check the start error. Examples remain deterministic under both normal scheduling and GOMAXPROCS=1: single-batch configs (MinItems==MaxItems) collapse the pipeline into one goroutine for the chained/per-batch-print examples, and Example_dynamicConfig counts items under a mutex and prints an invariant summary after <-b.Done() instead of racing per-batch output. Example_customConfig and Example_simpleProcessor-style helpers (RunBatchAndWait) were already API-compatible and are left as master's where appropriate. Verified: gofmt -l (clean), go vet, go build, golangci-lint (0 issues), go test -race, and go test -run Example -count=20 both normally and with GOMAXPROCS=1. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(batch): clarify EOF flush in error-handling example comment The batch holds 13 items (two source reads become errors), so it is flushed at end-of-input rather than by the MaxItems cap. Reword the comment so the lead clause doesn't suggest MaxItems triggers the flush. Suggested by gemini-code-assist on #69. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Summary
Makes
Batchsingle-use, changesBatch.Goto return(<-chan error, error), replaces the dedicated ID-generator goroutine with an inline per-reader counter, and fixes a timer leak inwaitForItems.Batch.Gonow returns(<-chan error, error). Start failures are reported via the returned error instead of a panic or the pipeline error channel:ErrBatchUsedErrNilSourceBoth are
errors.Is-able. On a start error the returned channel is non-nil and already closed, so ranging over it is always safe even if the error is ignored.Migration:
errs, err := b.Go(ctx, s, p...)and checkerr.Batchis single-use. A secondGo()on the same Batch returnsErrBatchUsed; create a new Batch withNewfor each run.Removed
BufferConfig.IDBufferSizeandDefaultIDBufferSize. Item IDs come from an inline counter — there is no ID-generator goroutine or channel. DropIDBufferSizefrom anyBufferConfigliteral.What it does & why
New()is cheap (it allocates no goroutines or channels untilGo), so reuse saved essentially nothing while creating a whole class of cross-run concurrency hazards. Single-use removes them by construction:doReader-local variable — no atomic, no lock, nothing to reason about;done/errs;Done()/Go()data race (aDone()call concurrent with a reusingGo()) becomes impossible.Go.Goalready returns a channel, so folding start failures into a returned error is friendlier for long-lived services and treats nil-source and reuse consistently — norecover()needed, and the pipeline error channel is no longer overloaded with "you misused the constructor" errors.waitForItemsusestime.NewTimer+defer Stop()+Reset()instead oftime.After.doReader.CollectErrorsdoc clarified — it blocks until the error channel closes, so there is no need to wait onDone()afterward.Tests & verification
single_use_test.gocoversErrBatchUsed,ErrNilSource, and success-returns-nil; the returned channel on a rejectedGois asserted closed/drainable.reuse_test.goremoved.BenchmarkBatchThroughputuses only the public API, so it runs onmastertoo.gofmt -l .clean ·go vet ./...clean ·go build ./...clean ·go test -race ./...green (including the example// Output:checks) ·GOOS=linux GOARCH=386 go buildclean (no atomic ⇒ no 32-bit alignment concern).🤖 Generated with Claude Code