-
Notifications
You must be signed in to change notification settings - Fork 471
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
base: master
Are you sure you want to change the base?
Changes from 4 commits
1568065
fe82be7
379ba6b
6c23ba3
00e99fe
86848f2
c261185
0922083
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -108,6 +108,35 @@ export async function updateRecentResponse( | |
getLogger().warn(`sam: unable to save response at key "${key}": %s`, err) | ||
} | ||
} | ||
|
||
const buildProcessMementoRootKey = 'samcli.build.processes' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's change this to something like const BUILD_PROCESS_MEMENTO_ROOT_KEY = 'samcli.build.processes'; and maybe put it in the samutils if it makes sense to. We can also do the same with the 'global' as we should limit the amount of constant strings we have There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The above format wouldn't work since eslint only allows camelCase or PascalCase formats. The constant is defined in the sam/utils file. Are you referring to a different utils file? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ideally, we'd put this in a constants.ts file, but if not we can disable eslint for this line. The convention is that constants should be in all caps. |
||
|
||
/** | ||
* 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, 'global', templatePath) !== undefined | ||
} | ||
|
||
/** | ||
* 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('Template is already being built', { code: 'BuildInProgress' }) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's say "This template is already being built" to make sure users know they are trying to build the same template again, and that they can build different templates simultaneously. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we make the error code an enum? I think we had a class with different error codes already right, let's add this one in with them There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Those are specific SAM CLI error patterns. I don't think it makes sense to add this one to them since this error doesn't come directly from SAM CLI. |
||
} | ||
} | ||
|
||
export async function registerTemplateBuild(templatePath: string) { | ||
await updateRecentResponse(buildProcessMementoRootKey, 'global', templatePath, 'true') | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we add error handling here to catch if something goes wrong in the process of registering a template build? I think currently, the error would just bubble up from the actual sam build command, but it could be the case that build command succeeds but the registering fails There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. updateRecentResponse() already handles that by logging the error. export async function updateRecentResponse(
mementoRootKey: string,
identifier: string,
key: string,
value: string | undefined
) {
try {
const root = globals.context.workspaceState.get(mementoRootKey, {} as Record<string, Record<string, string>>)
await globals.context.workspaceState.update(mementoRootKey, {
...root,
[identifier]: { ...root[identifier], [key]: value },
})
} catch (err) {
getLogger().warn(`sam: unable to save response at key "${key}": %s`, err)
}
} There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we capture the error so that we can see it in our Kibana dashboard? I don't think we see logger errors in our dashboard iirc |
||
} | ||
|
||
export async function unregisterTemplateBuild(templatePath: string) { | ||
await updateRecentResponse(buildProcessMementoRootKey, 'global', 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 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,6 +17,7 @@ import { | |
getBuildFlags, | ||
ParamsSource, | ||
runBuild, | ||
SamBuildResult, | ||
} from '../../../shared/sam/build' | ||
import { TreeNode } from '../../../shared/treeview/resourceTreeDataProvider' | ||
import { createWizardTester } from '../wizards/wizardTestUtils' | ||
|
@@ -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) => | ||
|
@@ -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, | ||
|
@@ -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', | ||
[ | ||
|
@@ -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, '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)) | ||
|
||
|
@@ -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))]) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
} | ||
|
||
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() | ||
} |
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" | ||
} |
There was a problem hiding this comment.
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:
aws-toolkit-vscode/packages/core/src/shared/utilities/cliUtils.ts
Lines 235 to 238 in 5bfb867