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

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

3 participants