Skip to content

Commit 09510a3

Browse files
authored
refactor(webview): avoid race conditions in constructors. (#5961)
## Problem In the constructor for `ApplicationComposerManager`, there is an async call (non-awaited). This call is modifying state within the object and therefore causes a race condition with any other function relying on this state. See https://github.com/aws/aws-toolkit-vscode/blob/81132884f4fb3319bda4be7d3d873265191f43ce/packages/core/src/applicationcomposer/webviewManager.ts#L30-L33 ~This is potentially related to a flaky test.~ The test is still failing, but this refactor is still a win. ## Solution - Implement an async `create` method with a private constructor. Move this pattern upward to enable async calls. --- <!--- REMINDER: Ensure that your PR meets the guidelines in CONTRIBUTING.md --> License: I confirm that my contribution is made under the terms of the Apache 2.0 license.
1 parent 43f5b85 commit 09510a3

File tree

4 files changed

+39
-23
lines changed

4 files changed

+39
-23
lines changed

packages/core/src/applicationcomposer/activation.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ import { openTemplateInComposerCommand } from './commands/openTemplateInComposer
1010
import { openInComposerDialogCommand } from './commands/openInComposerDialog'
1111

1212
export async function activate(extensionContext: vscode.ExtensionContext): Promise<void> {
13-
const visualizationManager = new ApplicationComposerManager(extensionContext)
13+
const visualizationManager = await ApplicationComposerManager.create(extensionContext)
1414

1515
extensionContext.subscriptions.push(
1616
openTemplateInComposerCommand.register(visualizationManager),

packages/core/src/applicationcomposer/composerWebview.ts

Lines changed: 20 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -5,37 +5,43 @@
55

66
import { join } from 'path'
77
import * as nls from 'vscode-nls'
8-
const localize = nls.loadMessageBundle()
98
import * as path from 'path'
109
import * as vscode from 'vscode'
1110
import { handleMessage } from './handleMessage'
1211
import { FileWatchInfo } from './types'
1312

13+
const localize = nls.loadMessageBundle()
14+
1415
export class ApplicationComposer {
1516
public readonly documentUri: vscode.Uri
16-
public webviewPanel: vscode.WebviewPanel
17+
public webviewPanel: vscode.WebviewPanel = {} as vscode.WebviewPanel
1718
protected readonly disposables: vscode.Disposable[] = []
1819
protected isPanelDisposed = false
1920
private readonly onVisualizationDisposeEmitter = new vscode.EventEmitter<void>()
2021
public workSpacePath: string
2122
public defaultTemplatePath: string
2223
public defaultTemplateName: string
23-
// fileWatches is used to monitor template file changes and achieve bi-direction sync
24-
public fileWatches: Record<string, FileWatchInfo>
25-
private getWebviewContent: () => string
2624

27-
public constructor(
25+
private constructor(
2826
textDocument: vscode.TextDocument,
29-
context: vscode.ExtensionContext,
30-
getWebviewContent: () => string
27+
private getWebviewContent: () => Promise<string>,
28+
public fileWatches: Record<string, FileWatchInfo> = {} // fileWatches is used to monitor template file changes and achieve bi-direction sync
3129
) {
3230
this.getWebviewContent = getWebviewContent
3331
this.documentUri = textDocument.uri
34-
this.webviewPanel = this.setupWebviewPanel(textDocument, context)
3532
this.workSpacePath = path.dirname(textDocument.uri.fsPath)
3633
this.defaultTemplatePath = textDocument.uri.fsPath
3734
this.defaultTemplateName = path.basename(this.defaultTemplatePath)
38-
this.fileWatches = {}
35+
}
36+
37+
public static async create(
38+
textDocument: vscode.TextDocument,
39+
context: vscode.ExtensionContext,
40+
getWebviewContent: () => Promise<string>
41+
) {
42+
const obj = new ApplicationComposer(textDocument, getWebviewContent)
43+
obj.webviewPanel = await obj.setupWebviewPanel(textDocument, context)
44+
return obj
3945
}
4046

4147
public get onVisualizationDisposeEvent(): vscode.Event<void> {
@@ -56,25 +62,25 @@ export class ApplicationComposer {
5662
if (!this.isPanelDisposed) {
5763
this.webviewPanel.dispose()
5864
const document = await vscode.workspace.openTextDocument(this.documentUri)
59-
this.webviewPanel = this.setupWebviewPanel(document, context)
65+
this.webviewPanel = await this.setupWebviewPanel(document, context)
6066
}
6167
}
6268

6369
protected getText(textDocument: vscode.TextDocument): string {
6470
return textDocument.getText()
6571
}
6672

67-
private setupWebviewPanel(
73+
private async setupWebviewPanel(
6874
textDocument: vscode.TextDocument,
6975
context: vscode.ExtensionContext
70-
): vscode.WebviewPanel {
76+
): Promise<vscode.WebviewPanel> {
7177
const documentUri = textDocument.uri
7278

7379
// Create and show panel
7480
const panel = this.createVisualizationWebviewPanel(documentUri, context)
7581

7682
// Set the initial html for the webpage
77-
panel.webview.html = this.getWebviewContent()
83+
panel.webview.html = await this.getWebviewContent()
7884

7985
// Handle messages from the webview
8086
this.disposables.push(

packages/core/src/applicationcomposer/webviewManager.ts

Lines changed: 17 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -23,13 +23,15 @@ export class ApplicationComposerManager {
2323
protected readonly name: string = 'ApplicationComposerManager'
2424

2525
protected readonly managedVisualizations = new Map<string, ApplicationComposer>()
26-
protected extensionContext: vscode.ExtensionContext
2726
protected webviewHtml?: string
2827
protected readonly logger = getLogger()
2928

30-
public constructor(extensionContext: vscode.ExtensionContext) {
31-
this.extensionContext = extensionContext
32-
void this.fetchWebviewHtml()
29+
private constructor(protected extensionContext: vscode.ExtensionContext) {}
30+
31+
public static async create(extensionContext: vscode.ExtensionContext): Promise<ApplicationComposerManager> {
32+
const obj = new ApplicationComposerManager(extensionContext)
33+
await obj.fetchWebviewHtml()
34+
return obj
3335
}
3436

3537
private async fetchWebviewHtml() {
@@ -41,7 +43,7 @@ export class ApplicationComposerManager {
4143
}
4244
}
4345

44-
private getWebviewContent = () => {
46+
private getWebviewContent = async () => {
4547
if (!this.webviewHtml) {
4648
void this.fetchWebviewHtml()
4749
return ''
@@ -76,7 +78,11 @@ export class ApplicationComposerManager {
7678

7779
// Existing visualization does not exist, construct new visualization
7880
try {
79-
const newVisualization = new ApplicationComposer(document, this.extensionContext, this.getWebviewContent)
81+
const newVisualization = await ApplicationComposer.create(
82+
document,
83+
this.extensionContext,
84+
this.getWebviewContent
85+
)
8086
this.handleNewVisualization(document.uri.fsPath, newVisualization)
8187

8288
if (vscode.version === '1.91.0') {
@@ -99,7 +105,11 @@ export class ApplicationComposerManager {
99105
const document = await vscode.workspace.openTextDocument({
100106
language: 'yaml',
101107
})
102-
const newVisualization = new ApplicationComposer(document, this.extensionContext, this.getWebviewContent)
108+
const newVisualization = await ApplicationComposer.create(
109+
document,
110+
this.extensionContext,
111+
this.getWebviewContent
112+
)
103113
this.handleNewVisualization(document.uri.fsPath, newVisualization)
104114

105115
return newVisualization.getPanel()

packages/core/src/test/applicationcomposer/utils.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ import { WebviewContext } from '../../applicationcomposer/types'
1010
import { MockDocument } from '../fake/fakeDocument'
1111

1212
export async function createTemplate() {
13-
const manager = new ApplicationComposerManager(globals.context)
13+
const manager = await ApplicationComposerManager.create(globals.context)
1414
const panel = await manager.createTemplate()
1515
assert.ok(panel)
1616
return panel

0 commit comments

Comments
 (0)