Repository navigation
feat: execute a request's invocations concurrently - #66
Conversation
0df41bb to
6ed2ab1
Compare
7868d54 to
5344a2e
Compare
Say that the cap is per request rather than per server, how to force serial execution with a cap of one, and drop the sentence about the default's derivation that read as confusing. Addresses review comments on #66. Assisted-by: Claude:claude-fable-5-1 Signed-off-by: Miroslav Bajtoš <oss@bajtos.net>
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
It introduces a fundamental concurrency behavior change in the server execution path, which warrants final human review despite strong test coverage.
Review effort: Lite
Findings: 2
Open (2)
What changed in this PR
This PR changes the HTTP server’s batch execution model so invocations within a single batch request can run concurrently, controlled by a new per-request concurrency cap. This aligns batch performance more closely with sending many single-invocation requests, while keeping response assembly deterministic.
Changes:
- Execute batch invocations concurrently with a per-request semaphore and
server.WithMaxConcurrency(n)(default 100; 0 disables; negative panics). - Preserve deterministic response construction by collecting per-invocation results by request index and merging after all goroutines complete.
- Add targeted tests/benchmarks and update public documentation to reflect the new concurrency semantics and handler expectations.
| File | Description |
|---|---|
| server/options.go | Adds DefaultMaxConcurrency and WithMaxConcurrency option wired into server config. |
| server/http.go | Implements concurrent ExecuteBatch with per-request semaphore and ordered merge. |
| server/http_test.go | Adjusts an existing test to be safe under concurrent handler execution. |
| server/concurrency_test.go | Adds concurrency behavior tests (barrier, cap enforcement, order preservation, panic on negative). |
| server/bench_test.go | Adds benchmarks comparing sequential vs concurrent batching and many single requests. |
| README.md | Updates user-facing docs to mention concurrent batch execution and configurability. |
| notes.md | Records the design rationale and operational model for per-request concurrency. |
| execution/execution.go | Documents handler concurrency expectations for batch execution. |
| execution/batch/batch.go | Updates package docs to reflect concurrency + ordering constraints. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4c917ee23d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
| // Each goroutine writes only its own slot, so the slice needs no lock and | ||
| // the merge below runs in request order. | ||
| results := make([]batchResult, len(req.Invocations())) | ||
| var wg sync.WaitGroup |
There was a problem hiding this comment.
Can we use errgroup with SetLimit instead?
There was a problem hiding this comment.
I asked Claude, it rejected that idea for three reasons:
- AGENTS.md keeps the dependency list to what we have today, and
golang.org/x/syncwould be a new module. SetLimithas the opposite conventions from our option: 0 blocks everyGocall and -1 means unlimited, so we would need a translation layer.- errgroup deliberately does not propagate panics from goroutines to
Wait(the comment in errgroup.go explains why), so it would not have helped with the panic boundary Codex found, which fix: recover panics from validation code #67 now handles in the dispatcher.
However, Go 1.27 added a new API that simplifies the pattern we have here, and we reworked the code to use sync.WaitGroup.Go, which removes the Add/Done boilerplate without a new dependency.
Say that the cap is per request rather than per server, how to force serial execution with a cap of one, and drop the sentence about the default's derivation that read as confusing. Addresses review comments on #66. Assisted-by: Claude:claude-fable-5-1 Signed-off-by: Miroslav Bajtoš <oss@bajtos.net>
4c917ee to
a71785c
Compare
Say that the cap is per request rather than per server, how to force serial execution with a cap of one, and drop the sentence about the default's derivation that read as confusing. Addresses review comments on #66. Assisted-by: Claude:claude-fable-5-1 Signed-off-by: Miroslav Bajtoš <oss@bajtos.net>
a71785c to
a46976c
Compare
Say that the cap is per request rather than per server, how to force serial execution with a cap of one, and drop the sentence about the default's derivation that read as confusing. Addresses review comments on #66. Assisted-by: Claude:claude-fable-5-1 Signed-off-by: Miroslav Bajtoš <oss@bajtos.net>
a46976c to
5ede701
Compare
The server ran the invocations of one request one after another, so a request carrying 100 invocations took longer than 100 requests carrying one each, and batching made large multipart uploads slower rather than faster. Run each invocation on its own goroutine, the way net/http runs each connection, admitted through a per-request semaphore the way HTTP/2 and QUIC servers cap concurrent streams per connection. WithMaxConcurrency sets the cap; the default of 100 matches quic-go and the RFC 9113 floor, zero removes the cap and a negative value panics. The semaphore is acquired before spawning, so an oversized request waits in the loop rather than as a pile of blocked goroutines. Receipts and metadata are collected into a slice indexed by request position and merged after every invocation finishes, so the response is the same regardless of which handler finished first and needs no lock. Dispatcher errors are joined and surface after all siblings have completed; they still fail the batch as before. Benchmark, one request of 100 invocations, 1ms sleeping handler: sequential 151ms, concurrent 4.6ms, against 4.5ms for 100 concurrent requests of one. The CPU-bound handler goes from 18.9ms to 3.7ms. Fixes FIL-1261 Assisted-by: Claude:claude-fable-5-1 Signed-off-by: Miroslav Bajtoš <oss@bajtos.net>
The sub-benchmark names spelled out the batch size, so measuring a different size meant editing four lines instead of one constant. Assisted-by: Claude:claude-fable-5-1 Signed-off-by: Miroslav Bajtoš <oss@bajtos.net>
Say that the cap is per request rather than per server, how to force serial execution with a cap of one, and drop the sentence about the default's derivation that read as confusing. Addresses review comments on #66. Assisted-by: Claude:claude-fable-5-1 Signed-off-by: Miroslav Bajtoš <oss@bajtos.net>
Signed-off-by: Miroslav Bajtoš <oss@bajtos.net>
The README and the ExecuteBatch comment read "up to WithMaxConcurrency at a time", as if the option were a number. Name DefaultMaxConcurrency and say the option changes it. While here, replace the manual Add/Done pair with WaitGroup.Go. Assisted-by: Claude:claude-fable-5-1 Signed-off-by: Miroslav Bajtoš <oss@bajtos.net>
051e406 to
a23cedf
Compare

Written by Claude.
The server ran the invocations of one request one after another, so a request carrying 100 invocations took longer than 100 requests carrying one each, and batching made large multipart uploads slower rather than faster.
Each invocation now runs on its own goroutine, the way net/http runs each connection, admitted through a per-request semaphore the way HTTP/2 and QUIC servers cap concurrent streams per connection.
server.WithMaxConcurrency(n)sets the cap. The default of 100 matches quic-go'sMaxIncomingStreamsand the floor RFC 9113 recommends; x/net/http2 uses 250. Zero removes the cap for deployments that bound concurrency at the listener, and a negative value panics. The semaphore is acquired before spawning, so an oversized request waits in the loop rather than as a pile of blocked goroutines.Receipts and metadata are collected into a slice indexed by request position and merged after every invocation finishes, so the response is the same regardless of which handler finished first and needs no lock. Dispatcher errors are joined and surface after all siblings complete; they still fail the batch as before.
Handlers must be safe to call concurrently within one request, as they already had to be across requests. The
HandlerFuncdoc, thebatchpackage doc, the README andnotes.mdsay so. piri's six RPC handlers were audited before this change: their shared state is stores, mutex-guarded caches and a singleflight group, and none reads a sibling's result.Fourth PR in the stack for FIL-1261, on top of #67, which it depends on: without the dispatcher recovering panics from handlers (#64) and from validation code (#67), a panic on one of these goroutines would crash the process.
Tests: a barrier handler that only releases once all invocations have entered, so the test deadlocks under sequential execution; cap enforcement with a cap of 2 and 5 invocations; the zero cap running 150 at once; the negative value panicking; response metadata preserved in request order when later invocations finish first. Run three times under the race detector. The
examplespackage could not run locally because the sandbox forbids binding a TCP port; everything else inmake cipassed.Left for the dependency bump in sprue and piri: sprue's
AcceptBatchcomment still says the node executes sequentially, and since sprue sends up to 1000 accepts per request piri may want a higher cap.Benchmark results
Benchmark on an M-series laptop, one request of 100 invocations:
One request of 1000 invocations:
Concurrent execution is about 80x faster than sequential for the I/O handler and about 9x faster for the CPU handler.
Stack created with GitHub Stacks CLI • Give Feedback 💬