Skip to content

Commit c349c6a

Browse files
author
Dave Bartolomeo
committed
Fix race condition when generating evaluator log summaries
The original code that logged the human-readable log summary generated the log asynchronously, which was a reasonable choice. When I added support for viewing and scanning logs, I didn't notice that the summary was being generated asynchronously, and wrote my code assuming that the summary was already on disk when I opened it to find where each relation's log started. The effect was that, depending on timing, the evaluation sometimes failed with an error popup complaining about not being able to open the log summary file. The fix is to _generate_ the log summary synchronously, but continue to _log_ it asynchronously.
1 parent 234b059 commit c349c6a

File tree

1 file changed

+38
-20
lines changed

1 file changed

+38
-20
lines changed

extensions/ql-vscode/src/run-queries.ts

Lines changed: 38 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,7 @@ import { compileDatabaseUpgradeSequence, hasNondestructiveUpgradeCapabilities, u
3636
import { ensureMetadataIsComplete } from './query-results';
3737
import { SELECT_QUERY_NAME } from './contextual/locationFinder';
3838
import { DecodedBqrsChunk } from './pure/bqrs-cli-types';
39-
import { getErrorMessage } from './pure/helpers-pure';
39+
import { asError, getErrorMessage } from './pure/helpers-pure';
4040
import { generateSummarySymbolsFile } from './log-insights/summary-parser';
4141

4242
/**
@@ -207,7 +207,10 @@ export class QueryEvaluationInfo {
207207
logPath: this.evalLogPath,
208208
});
209209
if (await this.hasEvalLog()) {
210-
this.displayHumanReadableLogSummary(queryInfo, qs);
210+
queryInfo.evalLogLocation = this.evalLogPath;
211+
queryInfo.evalLogSummaryLocation = await this.generateHumanReadableLogSummary(qs);
212+
void this.logEndSummary(queryInfo.evalLogSummaryLocation, qs); // Logged asynchrnously
213+
211214
if (config.isCanary()) { // Generate JSON summary for viewer.
212215
await qs.cliServer.generateJsonLogSummary(this.evalLogPath, this.jsonEvalLogSummaryPath);
213216
queryInfo.jsonEvalLogSummaryLocation = this.jsonEvalLogSummaryPath;
@@ -340,25 +343,40 @@ export class QueryEvaluationInfo {
340343
}
341344

342345
/**
343-
* Calls the appropriate CLI command to generate a human-readable log summary
344-
* and logs to the Query Server console and query log file.
346+
* Calls the appropriate CLI command to generate a human-readable log summary.
347+
* @param qs The query server client.
348+
* @returns The path to the log summary, or `undefined` if the summary could not be generated.
345349
*/
346-
displayHumanReadableLogSummary(queryInfo: LocalQueryInfo, qs: qsClient.QueryServerClient): void {
347-
queryInfo.evalLogLocation = this.evalLogPath;
348-
void qs.cliServer.generateLogSummary(this.evalLogPath, this.evalLogSummaryPath, this.evalLogEndSummaryPath)
349-
.then(() => {
350-
queryInfo.evalLogSummaryLocation = this.evalLogSummaryPath;
351-
fs.readFile(this.evalLogEndSummaryPath, (err, buffer) => {
352-
if (err) {
353-
throw new Error(`Could not read structured evaluator log end of summary file at ${this.evalLogEndSummaryPath}.`);
354-
}
355-
void qs.logger.log(' --- Evaluator Log Summary --- ', { additionalLogLocation: this.logPath });
356-
void qs.logger.log(buffer.toString(), { additionalLogLocation: this.logPath });
357-
});
358-
})
359-
.catch(err => {
360-
void showAndLogWarningMessage(`Failed to generate human-readable structured evaluator log summary. Reason: ${err.message}`);
361-
});
350+
private async generateHumanReadableLogSummary(qs: qsClient.QueryServerClient): Promise<string | undefined> {
351+
try {
352+
await qs.cliServer.generateLogSummary(this.evalLogPath, this.evalLogSummaryPath, this.evalLogEndSummaryPath);
353+
return this.evalLogSummaryPath;
354+
355+
} catch (e) {
356+
const err = asError(e);
357+
void showAndLogWarningMessage(`Failed to generate human-readable structured evaluator log summary. Reason: ${err.message}`);
358+
return undefined;
359+
}
360+
}
361+
362+
/**
363+
* Logs the end summary to the Output window and log file.
364+
* @param logSummaryPath Path to the human-readable log summary
365+
* @param qs The query server client.
366+
*/
367+
private async logEndSummary(logSummaryPath: string | undefined, qs: qsClient.QueryServerClient): Promise<void> {
368+
if (logSummaryPath === undefined) {
369+
// Failed to generate the log, so we don't expect an end summary either.
370+
return;
371+
}
372+
373+
try {
374+
const endSummaryContent = await fs.readFile(this.evalLogEndSummaryPath, 'utf-8');
375+
void qs.logger.log(' --- Evaluator Log Summary --- ', { additionalLogLocation: this.logPath });
376+
void qs.logger.log(endSummaryContent, { additionalLogLocation: this.logPath });
377+
} catch (e) {
378+
void showAndLogWarningMessage(`Could not read structured evaluator log end of summary file at ${this.evalLogEndSummaryPath}.`);
379+
}
362380
}
363381

364382
/**

0 commit comments

Comments
 (0)