Fix crash when there are multiple packages to install per code fix action (microsoft#56400)

sheetalkamat · web-flow · commit 29bb85722676 · 2023-11-15T16:13:27.000-08:00
diff --git a/src/jsTyping/types.ts b/src/jsTyping/types.ts
@@ -51,6 +51,7 @@ export interface InstallPackageRequest extends TypingInstallerRequestWithProject
     readonly fileName: Path;
     readonly packageName: string;
     readonly projectRootPath: Path;
+    readonly id: number;
 }
 
 /** @internal */
@@ -61,6 +62,7 @@ export interface TypesRegistryResponse extends TypingInstallerResponse {
 
 export interface PackageInstalledResponse extends ProjectResponse {
     readonly kind: ActionPackageInstalled;
+    readonly id: number;
     readonly success: boolean;
     readonly message: string;
 }
diff --git a/src/server/typingInstallerAdapter.ts b/src/server/typingInstallerAdapter.ts
@@ -46,6 +46,11 @@ export interface TypingsInstallerWorkerProcess {
     send<T extends TypingInstallerRequestUnion>(rq: T): void;
 }
 
+interface PackageInstallPromise {
+    resolve(value: ApplyCodeActionCommandResult): void;
+    reject(reason: unknown): void;
+}
+
 /** @internal */
 export abstract class TypingsInstallerAdapter implements ITypingsInstaller {
     protected installer!: TypingsInstallerWorkerProcess;
@@ -63,10 +68,8 @@ export abstract class TypingsInstallerAdapter implements ITypingsInstaller {
     // It would be preferable to base our limit on the amount of space left in the
     // buffer, but we have yet to find a way to retrieve that value.
     private static readonly requestDelayMillis = 100;
-    private packageInstalledPromise: {
-        resolve(value: ApplyCodeActionCommandResult): void;
-        reject(reason: unknown): void;
-    } | undefined;
+    private packageInstalledPromise: Map<number, PackageInstallPromise> | undefined;
+    private packageInstallId = 0;
 
     constructor(
         protected readonly telemetryEnabled: boolean,
@@ -92,11 +95,13 @@ export abstract class TypingsInstallerAdapter implements ITypingsInstaller {
     }
 
     installPackage(options: InstallPackageOptionsWithProject): Promise<ApplyCodeActionCommandResult> {
-        this.installer.send<InstallPackageRequest>({ kind: "installPackage", ...options });
-        Debug.assert(this.packageInstalledPromise === undefined);
-        return new Promise<ApplyCodeActionCommandResult>((resolve, reject) => {
-            this.packageInstalledPromise = { resolve, reject };
+        this.packageInstallId++;
+        const request: InstallPackageRequest = { kind: "installPackage", ...options, id: this.packageInstallId };
+        const promise = new Promise<ApplyCodeActionCommandResult>((resolve, reject) => {
+            (this.packageInstalledPromise ??= new Map()).set(this.packageInstallId, { resolve, reject });
         });
+        this.installer.send(request);
+        return promise;
     }
 
     attach(projectService: ProjectService) {
@@ -136,15 +141,15 @@ export abstract class TypingsInstallerAdapter implements ITypingsInstaller {
                 this.typesRegistryCache = new Map(Object.entries(response.typesRegistry));
                 break;
             case ActionPackageInstalled: {
-                const { success, message } = response;
-                if (success) {
-                    this.packageInstalledPromise!.resolve({ successMessage: message });
+                const promise = this.packageInstalledPromise?.get(response.id);
+                Debug.assertIsDefined(promise, "Should find the promise for package install");
+                this.packageInstalledPromise?.delete(response.id);
+                if (response.success) {
+                    promise.resolve({ successMessage: response.message });
                 }
                 else {
-                    this.packageInstalledPromise!.reject(message);
+                    promise.reject(response.message);
                 }
-                this.packageInstalledPromise = undefined;
-
                 this.projectService.updateTypingsForProject(response);
 
                 // The behavior is the same as for setTypings, so send the same event.
diff --git a/src/testRunner/tests.ts b/src/testRunner/tests.ts
@@ -140,6 +140,7 @@ import "./unittests/tsserver/autoImportProvider";
 import "./unittests/tsserver/auxiliaryProject";
 import "./unittests/tsserver/cachingFileSystemInformation";
 import "./unittests/tsserver/cancellationToken";
+import "./unittests/tsserver/codeFix";
 import "./unittests/tsserver/compileOnSave";
 import "./unittests/tsserver/completions";
 import "./unittests/tsserver/completionsIncomplete";
diff --git a/src/testRunner/unittests/tsserver/codeFix.ts b/src/testRunner/unittests/tsserver/codeFix.ts
@@ -0,0 +1,65 @@
+import * as ts from "../../_namespaces/ts";
+import {
+    dedent,
+} from "../../_namespaces/Utils";
+import {
+    baselineTsserverLogs,
+    openFilesForSession,
+    TestSession,
+} from "../helpers/tsserver";
+import {
+    createServerHost,
+    libFile,
+} from "../helpers/virtualFileSystemWithWatch";
+
+describe("unittests:: tsserver:: codeFix::", () => {
+    function setup() {
+        const host = createServerHost({
+            "/home/src/projects/project/src/file.ts": dedent`
+                import * as os from "os";
+                import * as https from "https";
+                import * as vscode from "vscode";
+            `,
+            "/home/src/projects/project/tsconfig.json": "{ }",
+            "/home/src/projects/project/node_modules/vscode/index.js": "export const x = 10;",
+            [libFile.path]: libFile.content,
+        });
+        const session = new TestSession({ host, typesRegistry: ["vscode"] });
+        openFilesForSession(["/home/src/projects/project/src/file.ts"], session);
+        const actions = session.executeCommandSeq<ts.server.protocol.GetCombinedCodeFixRequest>({
+            command: ts.server.protocol.CommandTypes.GetCombinedCodeFix,
+            arguments: {
+                scope: { type: "file", args: { file: "/home/src/projects/project/src/file.ts" } },
+                fixId: "installTypesPackage",
+            },
+        }).response as ts.server.protocol.CombinedCodeActions;
+        return { host, session, actions };
+    }
+    it("install package", () => {
+        const { host, session, actions } = setup();
+        actions.commands?.forEach(command =>
+            session.executeCommandSeq<ts.server.protocol.ApplyCodeActionCommandRequest>({
+                command: ts.server.protocol.CommandTypes.ApplyCodeActionCommand,
+                arguments: {
+                    command,
+                },
+            })
+        );
+        host.runPendingInstalls();
+        baselineTsserverLogs("codeFix", "install package", session);
+    });
+
+    it("install package when serialized", () => {
+        const { host, session, actions } = setup();
+        actions.commands?.forEach(command => {
+            session.executeCommandSeq<ts.server.protocol.ApplyCodeActionCommandRequest>({
+                command: ts.server.protocol.CommandTypes.ApplyCodeActionCommand,
+                arguments: {
+                    command,
+                },
+            });
+            host.runPendingInstalls();
+        });
+        baselineTsserverLogs("codeFix", "install package when serialized", session);
+    });
+});
diff --git a/src/typingsInstallerCore/typingsInstaller.ts b/src/typingsInstallerCore/typingsInstaller.ts
@@ -233,7 +233,7 @@ export abstract class TypingsInstaller {
 
     /** @internal */
     installPackage(req: InstallPackageRequest) {
-        const { fileName, packageName, projectName, projectRootPath } = req;
+        const { fileName, packageName, projectName, projectRootPath, id } = req;
         const cwd = forEachAncestorDirectory(getDirectoryPath(fileName), directory => {
             if (this.installTypingHost.fileExists(combinePaths(directory, "package.json"))) {
                 return directory;
@@ -247,6 +247,7 @@ export abstract class TypingsInstaller {
                 const response: PackageInstalledResponse = {
                     kind: ActionPackageInstalled,
                     projectName,
+                    id,
                     success,
                     message,
                 };
@@ -257,6 +258,7 @@ export abstract class TypingsInstaller {
             const response: PackageInstalledResponse = {
                 kind: ActionPackageInstalled,
                 projectName,
+                id,
                 success: false,
                 message: "Could not determine a project root path.",
             };
diff --git a/tests/baselines/reference/api/typescript.d.ts b/tests/baselines/reference/api/typescript.d.ts
@@ -49,9 +49,11 @@ declare namespace ts {
             readonly fileName: Path;
             readonly packageName: string;
             readonly projectRootPath: Path;
+            readonly id: number;
         }
         interface PackageInstalledResponse extends ProjectResponse {
             readonly kind: ActionPackageInstalled;
+            readonly id: number;
             readonly success: boolean;
             readonly message: string;
         }
diff --git a/tests/baselines/reference/tsserver/codeFix/install-package-when-serialized.js b/tests/baselines/reference/tsserver/codeFix/install-package-when-serialized.js
diff --git a/tests/baselines/reference/tsserver/codeFix/install-package.js b/tests/baselines/reference/tsserver/codeFix/install-package.js

Original file line number	Diff line number	Diff line change
`@@ -51,6 +51,7 @@ export interface InstallPackageRequest extends TypingInstallerRequestWithProject`
`51`	`51`	`readonly fileName: Path;`
`52`	`52`	`readonly packageName: string;`
`53`	`53`	`readonly projectRootPath: Path;`
	`54`	`+ readonly id: number;`
`54`	`55`	`}`
`55`	`56`
`56`	`57`	`/** @internal */`
`@@ -61,6 +62,7 @@ export interface TypesRegistryResponse extends TypingInstallerResponse {`
`61`	`62`
`62`	`63`	`export interface PackageInstalledResponse extends ProjectResponse {`
`63`	`64`	`readonly kind: ActionPackageInstalled;`
	`65`	`+ readonly id: number;`
`64`	`66`	`readonly success: boolean;`
`65`	`67`	`readonly message: string;`
`66`	`68`	`}`
Original file line number	Diff line number	Diff line change
`@@ -49,9 +49,11 @@ declare namespace ts {`
`49`	`49`	`readonly fileName: Path;`
`50`	`50`	`readonly packageName: string;`
`51`	`51`	`readonly projectRootPath: Path;`
	`52`	`+ readonly id: number;`
`52`	`53`	`}`
`53`	`54`	`interface PackageInstalledResponse extends ProjectResponse {`
`54`	`55`	`readonly kind: ActionPackageInstalled;`
	`56`	`+ readonly id: number;`
`55`	`57`	`readonly success: boolean;`
`56`	`58`	`readonly message: string;`
`57`	`59`	`}`