Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(appbuilder): Prevent parallel build processes on the same template #6172

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,7 @@

import { samSyncUrl } from '../../../shared/constants'
import { CloudFormationTemplateRegistry } from '../../../shared/fs/templateRegistry'
import { syncMementoRootKey } from '../../../shared/sam/sync'

import { syncMementoRootKey } from '../../../shared/sam/constants'
import { createExitPrompter } from '../../../shared/ui/common/exitPrompter'
import { createTemplatePrompter, TemplateItem } from '../../../shared/ui/sam/templatePrompter'
import { Wizard } from '../../../shared/wizards/wizard'
Expand Down
40 changes: 29 additions & 11 deletions packages/core/src/shared/sam/build.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,12 +25,15 @@ import {
getSamCliPathAndVersion,
getTerminalFromError,
isDotnetRuntime,
registerTemplateBuild,
throwIfTemplateIsBeingBuilt,
unregisterTemplateBuild,
updateRecentResponse,
} from './utils'
import { getConfigFileUri, validateSamBuildConfig } from './config'
import { runInTerminal } from './processTerminal'
import { buildMementoRootKey } from './constants'

const buildMementoRootKey = 'samcli.build.params'
export interface BuildParams {
readonly template: TemplateItem
readonly projectRoot: vscode.Uri
Expand Down Expand Up @@ -207,6 +210,9 @@ export async function runBuild(arg?: TreeNode): Promise<SamBuildResult> {
throw new CancellationError('user')
}

throwIfTemplateIsBeingBuilt(params.template.uri.path)
await registerTemplateBuild(params.template.uri.path)

const projectRoot = params.projectRoot

const defaultFlags: string[] = ['--cached', '--parallel', '--save-params', '--use-container']
Expand All @@ -229,23 +235,35 @@ export async function runBuild(arg?: TreeNode): Promise<SamBuildResult> {
await updateRecentResponse(buildMementoRootKey, 'global', 'templatePath', templatePath)

try {
const { path: samCliPath } = await getSamCliPathAndVersion()
await vscode.window.withProgress(
{
location: vscode.ProgressLocation.Notification,
},
async (progress) => {
progress.report({ message: `Building SAM template at ${params.template.uri.path}` })
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we also add a cancel button to it? you can refer the implementation here:

const progress = await showMessageWithCancel(
localize('AWS.cli.installProgress', 'Installing: {0} CLI', cliToInstall.name),
timeout
)


// Create a child process to run the SAM build command
const buildProcess = new ChildProcess(samCliPath, ['build', ...buildFlags], {
spawnOptions: await addTelemetryEnvVar({
cwd: params.projectRoot.fsPath,
env: await getSpawnEnv(process.env),
}),
})
const { path: samCliPath } = await getSamCliPathAndVersion()

// Create a child process to run the SAM build command
const buildProcess = new ChildProcess(samCliPath, ['build', ...buildFlags], {
spawnOptions: await addTelemetryEnvVar({
cwd: params.projectRoot.fsPath,
env: await getSpawnEnv(process.env),
}),
})

// Run SAM build in Terminal
await runInTerminal(buildProcess, 'build')
}
)

// Run SAM build in Terminal
await runInTerminal(buildProcess, 'build')
await unregisterTemplateBuild(params.template.uri.path)

return {
isSuccess: true,
}
} catch (error) {
await unregisterTemplateBuild(params.template.uri.path)
throw ToolkitError.chain(error, 'Failed to build SAM template', {
details: { terminal: getTerminalFromError(error), ...resolveBuildArgConflict(buildFlags) },
code: getErrorCode(error),
Expand Down
10 changes: 10 additions & 0 deletions packages/core/src/shared/sam/constants.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
/*!
* Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
* SPDX-License-Identifier: Apache-2.0
*/

export const buildProcessMementoRootKey = 'samcli.build.processes'
export const globalIdentifier = 'global'
export const buildMementoRootKey = 'samcli.build.params'
export const deployMementoRootKey = 'samcli.deploy.params'
export const syncMementoRootKey = 'samcli.sync.params'
3 changes: 1 addition & 2 deletions packages/core/src/shared/sam/deploy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ import {
updateRecentResponse,
} from './utils'
import { runInTerminal } from './processTerminal'
import { deployMementoRootKey } from './constants'

export interface DeployParams {
readonly paramsSource: ParamsSource
Expand All @@ -48,8 +49,6 @@ export interface DeployParams {
[key: string]: any
}

const deployMementoRootKey = 'samcli.deploy.params'

function getRecentDeployParams(identifier: string, key: string): string | undefined {
return getRecentResponse(deployMementoRootKey, identifier, key)
}
Expand Down
3 changes: 1 addition & 2 deletions packages/core/src/shared/sam/sync.ts
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ import { ParamsSource, createSyncParamsSourcePrompter } from '../ui/sam/paramsSo
import { createEcrPrompter } from '../ui/sam/ecrPrompter'
import { BucketSource, createBucketNamePrompter, createBucketSourcePrompter } from '../ui/sam/bucketPrompter'
import { runInTerminal } from './processTerminal'
import { syncMementoRootKey } from './constants'

export interface SyncParams {
readonly paramsSource: ParamsSource
Expand All @@ -68,8 +69,6 @@ export interface SyncParams {
readonly syncFlags?: string
}

export const syncMementoRootKey = 'samcli.sync.params'

// TODO: hook this up so it prompts the user when more than 1 environment is present in `samconfig.toml`
export function createEnvironmentPrompter(config: SamConfig, environments = config.listEnvironments()) {
const recentEnvironmentName = getRecentResponse(syncMementoRootKey, config.location.fsPath, 'environmentName')
Expand Down
28 changes: 28 additions & 0 deletions packages/core/src/shared/sam/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import { telemetry } from '../telemetry/telemetry'
import globals from '../extensionGlobals'
import { getLogger } from '../logger/logger'
import { ChildProcessResult } from '../utilities/processUtils'
import { buildProcessMementoRootKey, globalIdentifier } from './constants'

/**
* @description determines the root directory of the project given Template Item
Expand Down Expand Up @@ -108,6 +109,33 @@ export async function updateRecentResponse(
getLogger().warn(`sam: unable to save response at key "${key}": %s`, err)
}
}

/**
* Returns true if there's an ongoing build process for the provided template, false otherwise
* @Param templatePath The path to the template.yaml file
*/
function isBuildInProgress(templatePath: string): boolean {
return getRecentResponse(buildProcessMementoRootKey, globalIdentifier, templatePath) !== undefined
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Future note: these functions (and probably more, I guess) are using shared mutable state. You may want to consider wrapping this in a dedicated file similar to the existing packages/core/src/shared/globalState.ts , except for "workspace" state. That makes it clear when callers are using this global state. And it collects the pattern in one place, instead of many spread-out functions.

}

/**
* Throws an error if there's a build in progress for the provided template
* @Param templatePath The path to the template.yaml file
*/
export function throwIfTemplateIsBeingBuilt(templatePath: string) {
if (isBuildInProgress(templatePath)) {
throw new ToolkitError('This template is already being built', { code: 'BuildInProgress' })
Copy link
Member

@roger-zhangg roger-zhangg Dec 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would say this check is too strict. There are possibility that the build is finished or exited but we also failed to unregisterTemplateBuild or even toolkit maybe reload itself during the build process. In this case the user will never able to build the template again. One possible solution is to add a timeout to this cache. Like 2 minutes or 5 minutes. In this way, if the worst case happens ( build finished but we failed to update memeto) the user will still be able to build after time out is reached.

}
}

export async function registerTemplateBuild(templatePath: string) {
await updateRecentResponse(buildProcessMementoRootKey, globalIdentifier, templatePath, 'true')
}

export async function unregisterTemplateBuild(templatePath: string) {
await updateRecentResponse(buildProcessMementoRootKey, globalIdentifier, templatePath, undefined)
}

export function getSamCliErrorMessage(stderr: string): string {
// Split the stderr string by newline, filter out empty lines, and get the last line
const lines = stderr
Expand Down
65 changes: 49 additions & 16 deletions packages/core/src/test/shared/sam/build.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import {
getBuildFlags,
ParamsSource,
runBuild,
SamBuildResult,
} from '../../../shared/sam/build'
import { TreeNode } from '../../../shared/treeview/resourceTreeDataProvider'
import { createWizardTester } from '../wizards/wizardTestUtils'
Expand All @@ -32,6 +33,7 @@ import { samconfigCompleteData, validTemplateData } from './samTestUtils'
import { CloudFormationTemplateRegistry } from '../../../shared/fs/templateRegistry'
import { getTestWindow } from '../vscode/window'
import { CancellationError } from '../../../shared/utilities/timeoutUtils'
import { SamAppLocation } from '../../../awsService/appBuilder/explorer/samProject'

describe('SAM BuildWizard', async function () {
const createTester = async (params?: Partial<BuildParams>, arg?: TreeNode | undefined) =>
Expand Down Expand Up @@ -388,22 +390,8 @@ describe('SAM runBuild', () => {
verifyCorrectDependencyCall()
})

it('[entry: appbuilder node] with default flags should instantiate correct process in terminal', async () => {
const prompterTester = PrompterTester.init()
.handleQuickPick('Specify parameter source for build', async (quickPick) => {
// Need sometime to wait for the template to search for template file
await quickPick.untilReady()

assert.strictEqual(quickPick.items.length, 2)
const items = quickPick.items
assert.strictEqual(quickPick.items.length, 2)
assert.deepStrictEqual(items[0], { data: ParamsSource.Specify, label: 'Specify build flags' })
assert.deepStrictEqual(items[1].label, 'Use default values')
quickPick.acceptItem(quickPick.items[1])
})
.build()

// Invoke sync command from command palette
it('[entry: appbuilder node] with default flags should instantiate correct process in terminal and show progress notification', async () => {
const prompterTester = getPrompterTester()
const expectedSamAppLocation = {
workspaceFolder: workspaceFolder,
samTemplateUri: templateFile,
Expand All @@ -412,6 +400,10 @@ describe('SAM runBuild', () => {

await runBuild(new AppNode(expectedSamAppLocation))

getTestWindow()
.getFirstMessage()
.assertProgress(`Building SAM template at ${expectedSamAppLocation.samTemplateUri.path}`)

assert.deepEqual(mockChildProcessClass.getCall(0).args, [
'sam-cli-path',
[
Expand All @@ -437,6 +429,27 @@ describe('SAM runBuild', () => {
prompterTester.assertCallAll()
})

it('[entry: appbuilder node] should throw an error when running two build processes in parallel for the same template', async () => {
const prompterTester = getPrompterTester()
const expectedSamAppLocation = {
workspaceFolder: workspaceFolder,
samTemplateUri: templateFile,
projectRoot: projectRoot,
}
await assert.rejects(
async () => {
await runInParallel(expectedSamAppLocation)
},
(e: any) => {
assert.strictEqual(e instanceof ToolkitError, true)
assert.strictEqual(e.message, 'This template is already being built')
assert.strictEqual(e.code, 'BuildInProgress')
return true
}
)
prompterTester.assertCallAll(undefined, 2)
})

it('[entry: command palette] use samconfig should instantiate correct process in terminal', async () => {
const samconfigFile = vscode.Uri.file(await testFolder.write('samconfig.toml', samconfigCompleteData))

Expand Down Expand Up @@ -551,3 +564,23 @@ describe('SAM runBuild', () => {
})
})
})

async function runInParallel(samLocation: SamAppLocation): Promise<[SamBuildResult, SamBuildResult]> {
return Promise.all([runBuild(new AppNode(samLocation)), runBuild(new AppNode(samLocation))])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I worry this would cause some flakiness as the two build is run at same time. There might be a race condition that both Build run throwIfTemplateIsBeingBuilt both passes and then both run registerTemplateBuild and proceed to build. In this case the two build will be run together and cause test to fail. I would suggest we add a 0.05s wait time between them to avoid the unlikely but possible race condition.

}

function getPrompterTester() {
return PrompterTester.init()
.handleQuickPick('Specify parameter source for build', async (quickPick) => {
// Need sometime to wait for the template to search for template file
await quickPick.untilReady()

assert.strictEqual(quickPick.items.length, 2)
const items = quickPick.items
assert.strictEqual(quickPick.items.length, 2)
assert.deepStrictEqual(items[0], { data: ParamsSource.Specify, label: 'Specify build flags' })
assert.deepStrictEqual(items[1].label, 'Use default values')
quickPick.acceptItem(quickPick.items[1])
})
.build()
}
2 changes: 1 addition & 1 deletion packages/core/src/testInteg/sam.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -700,7 +700,7 @@ describe('SAM Integration Tests', async function () {
describe('SAM Build', async function () {
let workspaceFolder: vscode.WorkspaceFolder
// We're testing only one case (python 3.10 (ZIP)) on the integration tests. More niche cases are handled as unit tests.
const scenarioIndex = 2
const scenarioIndex = 3
const scenario = scenarios[scenarioIndex]
let testRoot: string
let sandbox: sinon.SinonSandbox
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
{
"type": "Bug Fix",
"description": "SAM build: prevent running multiple build processes for the same template"
}
Loading