Make functions async + other review comments

Author: shati-patel

extensions/ql-vscode/src/extension.ts

@@ -155,7 +155,7 @@ export async function activate(ctx: ExtensionContext): Promise<CodeQLExtensionIn
   }
 
   const distributionConfigListener = new DistributionConfigListener();
-  initializeLogging(ctx);
+  await initializeLogging(ctx);
   await initializeTelemetry(extension, ctx);
   languageSupport.install();
 
@@ -767,10 +767,10 @@ function getContextStoragePath(ctx: ExtensionContext) {
   return ctx.storagePath || ctx.globalStoragePath;
 }
 
-function initializeLogging(ctx: ExtensionContext): void {
+async function initializeLogging(ctx: ExtensionContext): Promise<void> {
   const storagePath = getContextStoragePath(ctx);
-  logger.setLogStoragePath(storagePath, false);
-  ideServerLogger.setLogStoragePath(storagePath, false);
+  await logger.setLogStoragePath(storagePath, false);
+  await ideServerLogger.setLogStoragePath(storagePath, false);
   ctx.subscriptions.push(logger);
   ctx.subscriptions.push(queryServerLogger);
   ctx.subscriptions.push(ideServerLogger);

extensions/ql-vscode/src/logging.ts

@@ -28,9 +28,16 @@ export interface Logger {
   removeAdditionalLogLocation(location: string | undefined): void;
 
   /**
-   * The base location location where all side log files are stored.
+   * The base location where all side log files are stored.
    */
   getBaseLocation(): string | undefined;
+
+  /**
+   * Sets the location where logs are stored.
+   * @param storagePath The path where logs are stored.
+   * @param isCustomLogDirectory Whether the logs are stored in a custom, user-specified directory.
+   */
+  setLogStoragePath(storagePath: string, isCustomLogDirectory: boolean): Promise<void>;
 }
 
 export type ProgressReporter = Progress<{ message: string }>;
@@ -49,14 +56,14 @@ export class OutputChannelLogger extends DisposableObject implements Logger {
     this.isCustomLogDirectory = false;
   }
 
-  setLogStoragePath(storagePath: string, isCustomLogDirectory: boolean): void {
+  async setLogStoragePath(storagePath: string, isCustomLogDirectory: boolean): Promise<void> {
     this.additionalLogLocationPath = path.join(storagePath, this.title);
 
     this.isCustomLogDirectory = isCustomLogDirectory;
 
     if (!this.isCustomLogDirectory) {
       // clear out any old state from previous runs
-      fs.remove(this.additionalLogLocationPath);
+      await fs.remove(this.additionalLogLocationPath);
     }
   }
 

extensions/ql-vscode/src/queryserver-client.ts

@@ -5,7 +5,7 @@ import { Disposable, CancellationToken, commands } from 'vscode';
 import { createMessageConnection, MessageConnection, RequestType } from 'vscode-jsonrpc';
 import * as cli from './cli';
 import { QueryServerConfig } from './config';
-import { Logger, ProgressReporter, queryServerLogger } from './logging';
+import { Logger, ProgressReporter } from './logging';
 import { completeQuery, EvaluationResult, progress, ProgressMessage, WithProgressId } from './pure/messages';
 import * as messages from './pure/messages';
 import { ProgressCallback, ProgressTask } from './commandRunner';
@@ -89,26 +89,28 @@ export class QueryServerClient extends DisposableObject {
     this.evaluationResultCallbacks = {};
   }
 
-  initLogger() {
+  async initLogger() {
     let storagePath = this.opts.contextStoragePath;
     let isCustomLogDirectory = false;
     if (this.config.customLogDirectory) {
       try {
-        fs.mkdirSync(this.config.customLogDirectory);
-        helpers.showAndLogInformationMessage(`Storing query server logs to user-specified directory: ${this.config.customLogDirectory}.`);
+        if (!(await fs.pathExists(this.config.customLogDirectory))) {
+          await fs.mkdir(this.config.customLogDirectory);
+        }
+        this.logger.log(`Saving query server logs to user-specified directory: ${this.config.customLogDirectory}.`);
         storagePath = this.config.customLogDirectory;
         isCustomLogDirectory = true;
       } catch (e) {
         helpers.showAndLogErrorMessage(`${this.config.customLogDirectory} is not a valid directory. Logs will be stored in a temporary workspace directory instead.`);
       }
     }
 
-    queryServerLogger.setLogStoragePath(storagePath, isCustomLogDirectory);
+    await this.logger.setLogStoragePath(storagePath, isCustomLogDirectory);
 
   }
 
   get logger(): Logger {
-    return queryServerLogger;
+    return this.opts.logger;
   }
 
   /** Stops the query server by disposing of the current server process. */
@@ -148,6 +150,7 @@ export class QueryServerClient extends DisposableObject {
 
   /** Starts a new query server process, sending progress messages to the given reporter. */
   private async startQueryServerImpl(progressReporter: ProgressReporter): Promise<void> {
+    await this.initLogger();
     const ramArgs = await this.cliServer.resolveRam(this.config.queryMemoryMb, progressReporter);
     const args = ['--threads', this.config.numThreads.toString()].concat(ramArgs);
 
@@ -172,7 +175,6 @@ export class QueryServerClient extends DisposableObject {
       args.push('-J=-agentlib:jdwp=transport=dt_socket,address=localhost:9010,server=y,suspend=n,quiet=y');
     }
 
-    this.initLogger();
     const child = cli.spawnServer(
       this.config.codeQlPath,
       'CodeQL query server',

extensions/ql-vscode/test/pure-tests/logging.test.ts

@@ -48,7 +48,7 @@ describe('OutputChannelLogger tests', () => {
   });
 
   it('should create a side log in the workspace area', async () => {
-    logger.setLogStoragePath(tempFolders.storagePath.name, false);
+    await logger.setLogStoragePath(tempFolders.storagePath.name, false);
 
     await logger.log('xxx', { additionalLogLocation: 'first' });
     await logger.log('yyy', { additionalLogLocation: 'second' });
@@ -65,7 +65,7 @@ describe('OutputChannelLogger tests', () => {
   });
 
   it('should delete side logs on dispose', async () => {
-    logger.setLogStoragePath(tempFolders.storagePath.name, false);
+    await logger.setLogStoragePath(tempFolders.storagePath.name, false);
     await logger.log('xxx', { additionalLogLocation: 'first' });
     await logger.log('yyy', { additionalLogLocation: 'second' });
 
@@ -80,7 +80,7 @@ describe('OutputChannelLogger tests', () => {
   });
 
   it('should not delete side logs on dispose in a custom directory', async () => {
-    logger.setLogStoragePath(tempFolders.storagePath.name, true);
+    await logger.setLogStoragePath(tempFolders.storagePath.name, true);
     await logger.log('xxx', { additionalLogLocation: 'first' });
     await logger.log('yyy', { additionalLogLocation: 'second' });
 
@@ -95,7 +95,7 @@ describe('OutputChannelLogger tests', () => {
   });
 
   it('should remove an additional log location', async () => {
-    logger.setLogStoragePath(tempFolders.storagePath.name, false);
+    await logger.setLogStoragePath(tempFolders.storagePath.name, false);
     await logger.log('xxx', { additionalLogLocation: 'first' });
     await logger.log('yyy', { additionalLogLocation: 'second' });
 
@@ -110,7 +110,7 @@ describe('OutputChannelLogger tests', () => {
   });
 
   it('should not remove an additional log location in a custom directory', async () => {
-    logger.setLogStoragePath(tempFolders.storagePath.name, true);
+    await logger.setLogStoragePath(tempFolders.storagePath.name, true);
     await logger.log('xxx', { additionalLogLocation: 'first' });
     await logger.log('yyy', { additionalLogLocation: 'second' });
 
@@ -126,17 +126,17 @@ describe('OutputChannelLogger tests', () => {
 
   it('should delete an existing folder when setting the log storage path', async () => {
     fs.createFileSync(path.join(tempFolders.storagePath.name, 'test-logger', 'xxx'));
-    logger.setLogStoragePath(tempFolders.storagePath.name, false);
+    await logger.setLogStoragePath(tempFolders.storagePath.name, false);
     // should be empty dir
 
+    await waitABit();
     const testLoggerFolder = path.join(tempFolders.storagePath.name, 'test-logger');
-    // TODO: Why does this test pass? I'd expect the length to be 0 if it's correctly deleted the existing folder.
-    expect(fs.readdirSync(testLoggerFolder).length).to.equal(1);
+    expect(fs.existsSync(testLoggerFolder)).to.be.false;
   });
 
   it('should not delete an existing folder when setting the log storage path for a custom directory', async () => {
     fs.createFileSync(path.join(tempFolders.storagePath.name, 'test-logger', 'xxx'));
-    logger.setLogStoragePath(tempFolders.storagePath.name, true);
+    await logger.setLogStoragePath(tempFolders.storagePath.name, true);
     // should not be empty dir
 
     const testLoggerFolder = path.join(tempFolders.storagePath.name, 'test-logger');