fix: don't buffer when extracting binary #12621
Conversation
packages/amplify-cli-npm/binary.ts
Outdated
| extract.on('entry', (header, extractStream, next) => { | ||
| if (header.type === 'file') { | ||
| extractStream.on('data', (chunk) => { | ||
| chunks.push(chunk); | ||
| const fileWriteStream = fs.createWriteStream(this.binaryPath, { | ||
| mode: 0o755, | ||
| }); | ||
| extractStream.pipe(fileWriteStream); | ||
| } | ||
| extractStream.on('end', () => { | ||
| next(); | ||
| }); | ||
|
|
||
| extractStream.resume(); | ||
| }); | ||
| extract.on('finish', () => { | ||
| if (chunks.length) { | ||
| const data = Buffer.concat(chunks); | ||
| fs.writeFileSync(this.binaryPath, data, { | ||
| mode: 0o755, | ||
| }); | ||
| } | ||
| }); | ||
| return extract; | ||
| } | ||
| } |
There was a problem hiding this comment.
there's strong assumption in this code that TAR has only one file.
In both old and new version.
| # limit memory for new processes to 1GB | ||
| # this is to make sure that install can work on small VMs | ||
| # i.e. not buffer content in memory while installing binary | ||
| ulimit -Sv 1000000 |
There was a problem hiding this comment.
12.0.0 release
After applying fix
packages/amplify-cli-npm/binary.ts
Outdated
| // Dummy array to collect a promise from nested pipeline that extracts tar content to a file. | ||
| const extractPromiseCollector: Array<Promise<void>> = []; | ||
| await pipeline(res.data, createGunzip(), this.extract(extractPromiseCollector)); | ||
| await extractPromiseCollector[0]; |
There was a problem hiding this comment.
why only await the first promise and not Promise.allSettled?
There was a problem hiding this comment.
if we get here there's only one promise, but Promise.all would do too.
There was a problem hiding this comment.
Changed. I used Promise.all not Promise.allSettled . This should throw if extracting fails.
Codecov Report
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. @@ Coverage Diff @@
## dev #12621 +/- ##
===========================================
+ Coverage 0.00% 47.58% +47.58%
===========================================
Files 1296 843 -453
Lines 149743 38364 -111379
Branches 1296 7846 +6550
===========================================
+ Hits 0 18256 +18256
+ Misses 148447 18477 -129970
- Partials 1296 1631 +335
... and 1281 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
There was a problem hiding this comment.
looks good, only nit would be to add a note why we had to go this route (spawnSync was executing prematurely because tar.extract callbacks were not being awaited; this approach collects those nested callbacks and forces the process to await all) or something like that
Description of changes
Pipes the file instead of buffering in memory at binary installation.
Issue #, if available
Description of how you validated changes
E2E tests. I.e.
amplify_sudo_install_testChecklist
yarn testpassesBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.