Add arbitrary API request batching - #63937
Conversation
There was a problem hiding this comment.
Pull request overview
Adds typed arbitrary request batching across the backend, sync API, and async API, including automatic tick-based batching.
Changes:
- Adds batch protocol handling with per-request results and errors.
- Exposes typed
batchRequestsAPIs. - Adds automatic and manually scoped async batching with tests.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
tsc/internal/api/session.go |
Executes batched requests. |
tsc/internal/api/session_batch_test.go |
Tests backend batching. |
tsc/internal/api/proto.go |
Defines batch protocol types. |
tools/gen-proto/main.go |
Supports additional generated types. |
packages/typescript/test/sync/api.test.ts |
Tests sync batching. |
packages/typescript/test/async/api.test.ts |
Tests async and contextual batching. |
packages/typescript/src/api/sync/api.ts |
Exposes sync batching. |
packages/typescript/src/api/proto.ts |
Adds typed request/response mappings. |
packages/typescript/src/api/proto.generated.ts |
Adds generated batch declarations. |
packages/typescript/src/api/async/client.ts |
Implements automatic batching. |
packages/typescript/src/api/async/api.ts |
Exposes async batching APIs. |
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
tsc/internal/api/session.go:905
HandleRequestexposes internal handler values that are not always the public wire result represented byAPIResponse. For example, boolean handlers returnfalsewith an error (so the error arm getsresult: falseinstead ofnull), sync binary handlers returnRawBinarythat becomes a base64 string when nested rather than aSourceFileResponse, andreleasereturns the internaltrueacknowledgement even thoughAPIMethodInfodeclaresvoid. Normalize batched results to the public protocol shape—at minimum clearing results on error, adapting binary-returning methods, and hiding internal acknowledgements—and cover the sync binary case.
response.Result, err = s.HandleRequest(ctx, string(request.Method), request.Params)
if err != nil {
response.Error = err.Error()
| const resultPromise = new Promise<APIMethodInfo[K]["result"]>((resolve, reject) => { | ||
| this.batchedRequests.push({ method, params, resolve, reject }); | ||
| this.scheduleImmediateBatch(); |
There was a problem hiding this comment.
Preexisting issue, but... probably real. cc Andrew Branch (@andrewbranch) - I don't think there's a guard to ensure only one logical continuation is in the "connecting" state, since connect/connectViaSpawn/connectViaSocket don't set some kind of "connection in progress" bit before they yield with a promise deferral.
There was a problem hiding this comment.
(And I'm actually sidestepping the problem in this design by deferring scheduleImmediateBatch a tick inside the promise, so the promise yielded by the connect call above will execute before this one, which is why the tests work just fine.)
| } | ||
|
|
||
| const requestType = new RequestType<unknown, BatchRequestsResponse, void>("batchRequests"); | ||
| const params: BatchRequestsParams = { requests: requests.map(request => ({ method: request.method, params: request.params })) }; |
There was a problem hiding this comment.
Is unbound batching a good idea here? Event with the more manual batching approach I have encountered instances where the payloads just became too big and I got errors like string is too big. Without some control over batch size or some way to send requests and receive responses still as individual request/responses rather than one single request/response, this mechanism is very likely to run into this issue.
There was a problem hiding this comment.
I can (and I guess am) add(ing) some automatic pagination based on the max JS string length, and later we can make that configurable if needed, but aren't v8 json strings like 1GB max nowadays (somewhere over a billion characters or around a half billion depending on recent v8 version/pointer compression)? Are you running node with substantially increased heap size, then? Like, I guess I can see how the batch request for all the ASTs for a multi-million line project is probably over a GB as a string, so I see your point - I'd just assume other things are problematic within node at that point, too. I'd assume you need something more like a streaming API at that scale (which the async API naturally lends itself to).
|
This PR looks very promising. We use the sync API. If we could get this type of request to be publicly exposed, or build it up the requests and then do all of them at once, I think it could work for use. Internally we use a library that uses |
Yep. I actually already have a followup from the
comment I made above that adds an API output that maps the |
And automatic tick-based batching for the
asyncAPI.The core new API across backend, sync, and async API clients is a
batchRequestsmethod which looks like:This is strongly typed, but these are raw API responses/methods (so you, eg, have to map symbol responses into actual client symbol objects yourself), so it's not the most friendly to use, particularly for things like symbol member lookups. Errors, additionally, are returned on a per-request-in-batch basis (unless the error is an unrecoverable panic, which can, in turn, reject all requests in a batch).
On top of this core API, the
asyncAPI exposes aapi.batchContext()function that you canusingto hold back all API requests into a batch until the context is disposed of, for example:This can deadlock if you
awaitan API response before you.dispose()of the context, but gives excellent control of exactly which requests you'd like to batch without giving up any API usability.However! In a simple case like the above, the even simpler:
would also usually be automatically batched by the tick-based auto-batching I've added - so manual batching is, imo, of somewhat limited use for the async API. Automatic batching is probably all an async API consumer will ever need.
For the
syncversion of the API, I do not currently have a "user-friendly" abstraction over the request batcher like I've added forasync, though I have some ideas for one if we think it'd be valuable to have - it's just a bit of work inpackages\typescript\scripts\generateSync.tsto add.Fixes #63903