Merge pull request #1554 from nagilson/nagilson-does-install-still-exist

Improve bugs with installKey management

vscode-dotnet-runtime-library/src/Acquisition/DotnetCoreAcquisitionWorker.ts

@@ -60,9 +60,12 @@ export class DotnetCoreAcquisitionWorker implements IDotnetCoreAcquisitionWorker
     private readonly dotnetExecutable: string;
     private globalResolver: GlobalInstallerResolver | null;
 
-    private acquisitionPromises: { [installKeys: string]: Promise<string> | undefined };
+    private acquisitionPromises: { [installKey: string]: Promise<string> | undefined };
     private extensionContext : IVSCodeExtensionContext;
 
+    // @member usingNoInstallInvoker - Only use this for test when using the No Install Invoker to fake the worker into thinking a path is on disk.
+    protected usingNoInstallInvoker = false;
+
     constructor(protected readonly context: IAcquisitionWorkerContext, private readonly utilityContext : IUtilityContext, extensionContext : IVSCodeExtensionContext) {
         const dotnetExtension = os.platform() === 'win32' ? '.exe' : '';
         this.dotnetExecutable = `dotnet${dotnetExtension}`;
@@ -79,8 +82,14 @@ export class DotnetCoreAcquisitionWorker implements IDotnetCoreAcquisitionWorker
 
         this.removeFolderRecursively(this.context.installDirectoryProvider.getStoragePath());
 
-        await this.context.extensionState.update(this.installingVersionsKey, []);
-        await this.context.extensionState.update(this.installedVersionsKey, []);
+        // This does not uninstall global things yet, so don't remove their keys.
+        const installingVersions = this.context.extensionState.get<string[]>(this.installingVersionsKey, []);
+        const remainingInstallingVersions = installingVersions.filter(x => this.isGlobalInstallKey(x));
+        await this.context.extensionState.update(this.installingVersionsKey, remainingInstallingVersions);
+
+        const installedVersions = this.context.extensionState.get<string[]>(this.installedVersionsKey, []);
+        const remainingInstalledVersions = installedVersions.filter(x => this.isGlobalInstallKey(x));
+        await this.context.extensionState.update(this.installedVersionsKey, remainingInstalledVersions);
 
         this.context.eventStream.post(new DotnetUninstallAllCompleted());
     }
@@ -100,6 +109,11 @@ export class DotnetCoreAcquisitionWorker implements IDotnetCoreAcquisitionWorker
         return this.acquire(await installerResolver.getFullySpecifiedVersion(), false, installerResolver);
     }
 
+    private isGlobalInstallKey(installKey : string) : boolean
+    {
+        return installKey.toLowerCase().includes('global');
+    }
+
     /**
      *
      * @remarks this is simply a wrapper around the acquire function.
@@ -109,7 +123,15 @@ export class DotnetCoreAcquisitionWorker implements IDotnetCoreAcquisitionWorker
         return this.acquire(version, true, undefined, invoker);
     }
 
-    public async acquireStatus(version: string, installRuntime: boolean, architecture? : string): Promise<IDotnetAcquireResult | undefined> {
+    /**
+     *
+     * @param version The version of the runtime or sdk to check
+     * @param installRuntime Whether this is a local runtime status check or a local SDK status check.
+     * @param architecture The architecture of the install. Undefined means it will be the default arch, which is the node platform arch.
+     * @returns The result of the install with the path to dotnet if installed, else undefined.
+     */
+    public async acquireStatus(version: string, installRuntime: boolean, architecture? : string): Promise<IDotnetAcquireResult | undefined>
+    {
         const installKey = DotnetCoreAcquisitionWorker.getInstallKeyCustomArchitecture(version, architecture ? architecture : this.installingArchitecture)
 
         const existingAcquisitionPromise = this.acquisitionPromises[installKey];
@@ -129,7 +151,7 @@ export class DotnetCoreAcquisitionWorker implements IDotnetCoreAcquisitionWorker
             installedVersions = await this.managePreinstalledVersion(dotnetInstallDir, installedVersions);
         }
 
-        if (installedVersions.includes(installKey) && fs.existsSync(dotnetPath))
+        if (installedVersions.includes(installKey) && (fs.existsSync(dotnetPath) || this.usingNoInstallInvoker ))
         {
             // Requested version has already been installed.
             this.context.eventStream.post(new DotnetAcquisitionStatusResolved(installKey, version));
@@ -170,6 +192,7 @@ export class DotnetCoreAcquisitionWorker implements IDotnetCoreAcquisitionWorker
                 Debugging.log(`The Acquisition Worker has Determined a Global Install was requested.`, this.context.eventStream);
 
                 acquisitionPromise = this.acquireGlobalCore(globalInstallerResolver, installKey).catch((error: Error) => {
+                    this.removeVersionFromExtensionState(this.installingVersionsKey, installKey);
                     delete this.acquisitionPromises[installKey];
                     throw new Error(`.NET Acquisition Failed: ${error.message}`);
                 });
@@ -179,11 +202,14 @@ export class DotnetCoreAcquisitionWorker implements IDotnetCoreAcquisitionWorker
                 Debugging.log(`The Acquisition Worker has Determined a Local Install was requested.`, this.context.eventStream);
 
                 acquisitionPromise = this.acquireLocalCore(version, installRuntime, installKey, localInvoker!).catch((error: Error) => {
+                    this.removeVersionFromExtensionState(this.installingVersionsKey, installKey);
                     delete this.acquisitionPromises[installKey];
                     throw new Error(`.NET Acquisition Failed: ${error.message}`);
                 });
             }
 
+            // Put this promise into the list so we can let other requests run at the same time
+            // Allows us to return the end result of this current request for any following duplicates while we are still running.
             this.acquisitionPromises[installKey] = acquisitionPromise;
             return acquisitionPromise.then((res) => ({ dotnetPath: res }));
         }
@@ -229,7 +255,7 @@ export class DotnetCoreAcquisitionWorker implements IDotnetCoreAcquisitionWorker
             installedVersions = await this.managePreinstalledVersion(dotnetInstallDir, installedVersions);
         }
 
-        if (installedVersions.includes(installKey) && fs.existsSync(dotnetPath)) {
+        if (installedVersions.includes(installKey) && (fs.existsSync(dotnetPath) || this.usingNoInstallInvoker)) {
             // Version requested has already been installed.
             this.context.installationValidator.validateDotnetInstall(installKey, dotnetPath);
             this.context.eventStream.post(new DotnetAcquisitionAlreadyInstalled(installKey,
@@ -261,6 +287,7 @@ export class DotnetCoreAcquisitionWorker implements IDotnetCoreAcquisitionWorker
         await this.removeVersionFromExtensionState(this.installingVersionsKey, installKey);
         await this.addVersionToExtensionState(this.installedVersionsKey, installKey);
 
+        delete this.acquisitionPromises[installKey];
         return dotnetPath;
     }
 
@@ -280,7 +307,7 @@ export class DotnetCoreAcquisitionWorker implements IDotnetCoreAcquisitionWorker
             }
             if(uninstallLocalSDK)
             {
-                await this.uninstallRuntimeOrSDK(installKey);
+                await this.uninstallLocalRuntimeOrSDK(installKey);
             }
         }
     }
@@ -330,6 +357,7 @@ export class DotnetCoreAcquisitionWorker implements IDotnetCoreAcquisitionWorker
         await this.addVersionToExtensionState(this.installedVersionsKey, installKey);
 
         this.context.eventStream.post(new DotnetGlobalAcquisitionCompletionEvent(`The version ${installKey} completed successfully.`));
+        delete this.acquisitionPromises[installKey];
         return installedSDKPath;
     }
 
@@ -366,7 +394,7 @@ export class DotnetCoreAcquisitionWorker implements IDotnetCoreAcquisitionWorker
             if(legacyInstall.includes(version))
             {
                 this.context.eventStream.post(new DotnetLegacyInstallRemovalRequestEvent(`Trying to remove legacy install: ${legacyInstall} of ${version}.`));
-                await this.uninstallRuntimeOrSDK(legacyInstall);
+                await this.uninstallLocalRuntimeOrSDK(legacyInstall);
             }
         }
     }
@@ -398,7 +426,7 @@ export class DotnetCoreAcquisitionWorker implements IDotnetCoreAcquisitionWorker
         {
             this.context.eventStream.post(new DotnetInstallGraveyardEvent(
                 `Attempting to remove .NET at ${installKey} again, as it was left in the graveyard.`));
-            await this.uninstallRuntimeOrSDK(installKey);
+            await this.uninstallLocalRuntimeOrSDK(installKey);
         }
     }
 
@@ -425,7 +453,13 @@ export class DotnetCoreAcquisitionWorker implements IDotnetCoreAcquisitionWorker
         await this.context.extensionState.update(this.installPathsGraveyardKey, graveyard);
     }
 
-    public async uninstallRuntimeOrSDK(installKey : string) {
+    public async uninstallLocalRuntimeOrSDK(installKey : string)
+    {
+        if(this.isGlobalInstallKey(installKey))
+        {
+            return;
+        }
+
         try
         {
             delete this.acquisitionPromises[installKey];

vscode-dotnet-runtime-library/src/Acquisition/LinuxVersionResolver.ts

@@ -175,9 +175,9 @@ export class LinuxVersionResolver
         {
             // Implement any custom logic for a Distro Class in a new DistroSDKProvider and add it to the factory here.
             case null:
-                const error = new DotnetAcquisitionDistroUnknownError(new Error(this.baseUnsupportedDistroErrorMessage), getInstallKeyFromContext(this.acquireContext));
-                this.acquisitionContext.eventStream.post(error);
-                throw error.error;
+                const unknownDistroErr = new DotnetAcquisitionDistroUnknownError(new Error(this.baseUnsupportedDistroErrorMessage), getInstallKeyFromContext(this.acquireContext));
+                this.acquisitionContext.eventStream.post(unknownDistroErr);
+                throw unknownDistroErr.error;
             case 'Red Hat Enterprise Linux':
                 if(this.isRedHatVersion7(distroAndVersion.version))
                 {

vscode-dotnet-runtime-library/src/test/mocks/MockObjects.ts

@@ -36,6 +36,7 @@ import { ITelemetryReporter } from '../../EventStream/TelemetryObserver';
 import { IUtilityContext } from '../../Utils/IUtilityContext';
 import { IVSCodeEnvironment } from '../../Utils/IVSCodeEnvironment';
 import { IDotnetAcquireResult } from '../../IDotnetAcquireResult';
+import { IDotnetCoreAcquisitionWorker } from '../../Acquisition/IDotnetCoreAcquisitionWorker';
 
 const testDefaultTimeoutTimeMs = 60000;
 /* tslint:disable:no-any */
@@ -81,6 +82,13 @@ export class NoInstallAcquisitionInvoker extends IAcquisitionInvoker {
 
         });
     }
+
+    constructor(eventStream : IEventStream, worker : MockDotnetCoreAcquisitionWorker)
+    {
+        super(eventStream);
+        worker.enableNoInstallInvoker();
+    }
+
 }
 
 export class MockDotnetCoreAcquisitionWorker extends DotnetCoreAcquisitionWorker
@@ -115,6 +123,11 @@ export class MockDotnetCoreAcquisitionWorker extends DotnetCoreAcquisitionWorker
         this.context.installingArchitecture = newArch;
         this.context.acquisitionContext!.architecture = newArch;
     }
+
+    public enableNoInstallInvoker()
+    {
+        this.usingNoInstallInvoker = true;
+    }
 }
 
 export class RejectingAcquisitionInvoker extends IAcquisitionInvoker {

vscode-dotnet-runtime-library/src/test/unit/DotnetCoreAcquisitionWorker.test.ts

@@ -44,7 +44,7 @@ suite('DotnetCoreAcquisitionWorker Unit Tests', function () {
         const context = new MockExtensionContext();
         const eventStream = new MockEventStream();
         const acquisitionWorker = getMockAcquisitionWorker(isRuntimeWorker, version, arch, eventStream, context);
-        const invoker = new NoInstallAcquisitionInvoker(eventStream);
+        const invoker = new NoInstallAcquisitionInvoker(eventStream, acquisitionWorker);
         return [acquisitionWorker, eventStream, context, invoker];
     }
 
@@ -140,7 +140,7 @@ suite('DotnetCoreAcquisitionWorker Unit Tests', function () {
         assert.exists(undefinedEvent, 'Undefined event exists');
 
         await acquisitionWorker.acquireSDK(version, invoker);
-        result = await acquisitionWorker.acquireStatus(version, false);
+        result = await acquisitionWorker.acquireStatus(version, false, undefined);
         await assertAcquisitionSucceeded(installKey, result!.dotnetPath, eventStream, context, false);
         const resolvedEvent = eventStream.events.find(event => event instanceof DotnetAcquisitionStatusResolved);
         assert.exists(resolvedEvent, 'The sdk is resolved');
@@ -155,8 +155,8 @@ suite('DotnetCoreAcquisitionWorker Unit Tests', function () {
         const undefinedEvent = eventStream.events.find(event => event instanceof DotnetAcquisitionStatusUndefined);
         assert.exists(undefinedEvent);
 
-        await acquisitionWorker.acquireSDK(version, invoker);
-        result = await acquisitionWorker.acquireStatus(version, true);
+        await acquisitionWorker.acquireRuntime(version, invoker);
+        result = await acquisitionWorker.acquireStatus(version, true, undefined);
         await assertAcquisitionSucceeded(installKey, result!.dotnetPath, eventStream, context, true);
         const resolvedEvent = eventStream.events.find(event => event instanceof DotnetAcquisitionStatusResolved);
         assert.exists(resolvedEvent);

vscode-dotnet-runtime-library/src/test/unit/WinMacGlobalInstaller.test.ts

@@ -7,7 +7,7 @@ import * as chai from 'chai';
 import * as os from 'os';
 import * as fs from 'fs';
 import * as path from 'path';
-import { MockCommandExecutor, MockEventStream, MockExtensionContext, MockFileUtilities, MockInstallationValidator, NoInstallAcquisitionInvoker } from '../mocks/MockObjects';
+import { MockCommandExecutor, MockFileUtilities } from '../mocks/MockObjects';
 import { WinMacGlobalInstaller } from '../../Acquisition/WinMacGlobalInstaller';
 import { FileUtilities } from '../../Utils/FileUtilities';
 import { getMockAcquisitionContext, getMockUtilityContext } from './TestUtility';

vscode-dotnet-sdk-extension/src/test/functional/DotnetCoreAcquisitionExtension.test.ts

@@ -160,7 +160,7 @@ suite('DotnetCoreAcquisitionExtension End to End', function()
     fs.writeFileSync(dotnetExePath, '');
 
     // Assert preinstalled SDKs are detected
-    const acquisitionInvoker = new NoInstallAcquisitionInvoker(eventStream);
+    const acquisitionInvoker = new NoInstallAcquisitionInvoker(eventStream, acquisitionWorker);
     const result = await acquisitionWorker.acquireSDK(version, acquisitionInvoker);
     assert.equal(path.dirname(result.dotnetPath), dotnetDir);
     const preinstallEvents = eventStream.events