Skip to content

Add arbitrary API request batching - #63937

Open
Wesley Wigham (weswigham) wants to merge 4 commits into
microsoft:mainfrom
weswigham:batching-api-requests
Open

Add arbitrary API request batching#63937
Wesley Wigham (weswigham) wants to merge 4 commits into
microsoft:mainfrom
weswigham:batching-api-requests

Conversation

@weswigham

Copy link
Copy Markdown
Member

And automatic tick-based batching for the async API.

The core new API across backend, sync, and async API clients is a batchRequests method which looks like:

const { responses: [parsedCommandLine, configFileResult] } = await api.batchRequests([
    { method: "parseCommandLine", params: { commandLine: ["--strict"] } },
    { method: "readConfigFile", params: { file: "/tsconfig.json" } },
]);

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 async API exposes a api.batchContext() function that you can using to hold back all API requests into a batch until the context is disposed of, for example:

const requests = await (async () => {
    using _ = api.batchContext();
    return [
        api.parseCommandLine(["--strict"]),
        api.readConfigFile("/tsconfig.json"),
    ] as const;
})();

const [commandLine, config] = await Promise.all(requests);

This can deadlock if you await an 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:

const [commandLine, config] = await Promise.all([
    api.parseCommandLine(["--strict"]),
    api.readConfigFile("/tsconfig.json"),
]);

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 sync version of the API, I do not currently have a "user-friendly" abstraction over the request batcher like I've added for async, though I have some ideas for one if we think it'd be valuable to have - it's just a bit of work in packages\typescript\scripts\generateSync.ts to add.

Fixes #63903

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 batchRequests APIs.
  • 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.

Comment thread packages/typescript/src/api/async/client.ts
Comment thread packages/typescript/src/api/proto.ts Outdated
Comment thread packages/typescript/src/api/async/client.ts

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  • HandleRequest exposes internal handler values that are not always the public wire result represented by APIResponse. For example, boolean handlers return false with an error (so the error arm gets result: false instead of null), sync binary handlers return RawBinary that becomes a base64 string when nested rather than a SourceFileResponse, and release returns the internal true acknowledgement even though APIMethodInfo declares void. 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()

Comment on lines +258 to +260
const resultPromise = new Promise<APIMethodInfo[K]["result"]>((resolve, reject) => {
this.batchedRequests.push({ method, params, resolve, reject });
this.scheduleImmediateBatch();

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(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 })) };

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@weswigham Wesley Wigham (weswigham) Aug 24, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

@dragomirtitian

Copy link
Copy Markdown
Contributor

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 yield* to do the batching, so the code looks relatively normal, except for the generator stuff. We batch per currently, but batching by all methods works as well.

@weswigham

Copy link
Copy Markdown
Member Author

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 yield* to do the batching, so the code looks relatively normal, except for the generator stuff. We batch per currently, but batching by all methods works as well.

Yep. I actually already have a followup from the

I have some ideas for one if we think it'd be valuable to have

comment I made above that adds an API output that maps the async function input into generators and adds a driver to the sync API that can drive those generator based APIs for batching - so it's basically as user-friendly as the async API, I'd just prefer to layer that over this at this point instead of making it part of it, since the output is large.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug

Projects

Status: Not started

Development

Successfully merging this pull request may close these issues.

[7.1 API] Add more batched methods to the API

4 participants