Skip to content

Commit 0ee53e6

Browse files
authored
fix: don't buffer when extracting binary (#12621)
* chore: pipe tar stream to file * chore: synchronize internal pipeline * chore: simulate small VM * chore: non sudo install test * chore: pr feedback * chore: pr feedback
1 parent 260723c commit 0ee53e6

File tree

2 files changed

+50
-13
lines changed

2 files changed

+50
-13
lines changed

.circleci/config.base.yml

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -458,6 +458,30 @@ jobs:
458458
unsetSudoNpmRegistryUrl
459459
amplify version
460460
461+
amplify_install_test:
462+
<<: *defaults
463+
steps:
464+
- restore_cache:
465+
key: amplify-cli-repo-{{ .Branch }}-{{ .Revision }}
466+
- restore_cache:
467+
key: amplify-verdaccio-cache-{{ .Branch }}-{{ .Revision }}
468+
- restore_cache:
469+
key: amplify-pkg-binaries-{{ .Branch }}-{{ .Revision }}
470+
- run:
471+
name: Start verdaccio and Install Amplify CLI
472+
command: |
473+
source .circleci/local_publish_helpers.sh
474+
startLocalRegistry "$(pwd)/.circleci/verdaccio.yaml"
475+
setNpmRegistryUrlToLocal
476+
changeNpmGlobalPath
477+
# limit memory for new processes to 1GB
478+
# this is to make sure that install can work on small VMs
479+
# i.e. not buffer content in memory while installing binary
480+
ulimit -Sv 1000000
481+
npm install -g @aws-amplify/cli
482+
unsetNpmRegistryUrl
483+
amplify version
484+
461485
amplify_e2e_tests_pkg:
462486
parameters:
463487
os:
@@ -1091,6 +1115,17 @@ workflows:
10911115
- /run-e2e-with-rc\/.*/
10921116
- /tagged-release\/.*/
10931117
- /run-e2e\/.*/
1118+
- amplify_install_test:
1119+
context: amplify-ecr-image-pull
1120+
requires:
1121+
- upload_pkg_binaries
1122+
filters:
1123+
branches:
1124+
only:
1125+
- dev
1126+
- /run-e2e-with-rc\/.*/
1127+
- /tagged-release\/.*/
1128+
- /run-e2e\/.*/
10941129
- cleanup_resources:
10951130
context:
10961131
- cleanup-resources
@@ -1269,6 +1304,7 @@ workflows:
12691304
- integration_test
12701305
- amplify_e2e_tests_pkg
12711306
- amplify_sudo_install_test
1307+
- amplify_install_test
12721308
- amplify_console_integration_tests
12731309
- amplify_migration_tests_v10
12741310
- amplify_migration_tests_v6

packages/amplify-cli-npm/binary.ts

Lines changed: 14 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -121,7 +121,14 @@ export class Binary {
121121
console.log(`Downloading release from ${getCompressedBinaryUrl()}`);
122122
try {
123123
const res = await axios({ url: getCompressedBinaryUrl(), responseType: 'stream' });
124-
await pipeline(res.data, createGunzip(), this.extract());
124+
// An array to collect a promises from nested pipeline that extracts tar content to a file.
125+
// The tar file to actual file on disk streaming is kicked off by asynchronous events
126+
// of extract step. So top level pipeline may complete before streaming is completed.
127+
// We capture a Promise from that process to await it before proceeding,
128+
// so that we don't call spawnSync prematurely before content streaming completes.
129+
const extractPromiseCollector: Array<Promise<void>> = [];
130+
await pipeline(res.data, createGunzip(), this.extract(extractPromiseCollector));
131+
await Promise.all(extractPromiseCollector);
125132

126133
console.log('amplify has been installed!');
127134
spawnSync(this.binaryPath, ['version'], { cwd: process.cwd(), stdio: 'inherit' });
@@ -151,29 +158,23 @@ export class Binary {
151158
*
152159
* @returns tar.Extract
153160
*/
154-
private extract(): tar.Extract {
161+
private extract(extractPromiseCollector: Array<Promise<void>>): tar.Extract {
155162
const extract = tar.extract();
156-
const chunks: Uint8Array[] = [];
157163
extract.on('entry', (header, extractStream, next) => {
158164
if (header.type === 'file') {
159-
extractStream.on('data', (chunk) => {
160-
chunks.push(chunk);
165+
const fileWriteStream = fs.createWriteStream(this.binaryPath, {
166+
mode: 0o755,
161167
});
168+
// pipe tar entry to file stream
169+
// and collect a promise so that top level process can await it
170+
extractPromiseCollector.push(pipeline(extractStream, fileWriteStream));
162171
}
163172
extractStream.on('end', () => {
164173
next();
165174
});
166175

167176
extractStream.resume();
168177
});
169-
extract.on('finish', () => {
170-
if (chunks.length) {
171-
const data = Buffer.concat(chunks);
172-
fs.writeFileSync(this.binaryPath, data, {
173-
mode: 0o755,
174-
});
175-
}
176-
});
177178
return extract;
178179
}
179180
}

0 commit comments

Comments
 (0)