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: reduce memory pressure while uploading files #13718

Merged
merged 5 commits into from
Apr 16, 2024
Merged
Show file tree
Hide file tree
Changes from 4 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
2 changes: 1 addition & 1 deletion packages/amplify-provider-awscloudformation/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@
"@types/deep-diff": "^1.0.0",
"@types/folder-hash": "^4.0.1",
"@types/lodash.throttle": "^4.1.6",
"@types/node": "^12.12.6",
"@types/node": "^18.16.0",
"@types/uuid": "^8.0.0",
"jest": "^29.5.0",
"typescript": "^4.9.5"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,15 @@ import { $TSAny, $TSContext, AmplifyError, AmplifyFault, stateManager } from '@a

import _ from 'lodash';

import fs from 'fs-extra';
import ora from 'ora';
import { ListObjectVersionsOutput, ListObjectVersionsRequest, ObjectIdentifier } from 'aws-sdk/clients/s3';
import { pagedAWSCall } from './paged-call';
import { loadConfiguration } from '../configuration-manager';
import aws from './aws';

const providerName = require('../constants').ProviderName;
const consumers = require('stream/consumers');
sobolk marked this conversation as resolved.
Show resolved Hide resolved

const { fileLogger } = require('../utils/aws-logger');

Expand Down Expand Up @@ -126,6 +128,14 @@ export class S3 {
spinner.start('Uploading files.');
}
logger('uploadFile.s3.upload', [others])();
const minChunkSize = 5 * 1024 * 1024; // 5 MB
if (augmentedS3Params.Body instanceof fs.ReadStream && fs.statSync(augmentedS3Params.Body.path).size <= minChunkSize) {
// Buffer small files to avoid memory leak.
sobolk marked this conversation as resolved.
Show resolved Hide resolved
// Previous implementation used s3.putObject for small uploads, but it didn't have retries, see https://github.com/aws-amplify/amplify-cli/pull/13493.
// On the other hand uploading small streams leads to memory leak, see https://github.com/aws/aws-sdk-js/issues/2552.
// Therefore, buffering small files ourselves seems to be middle ground between memory leak and loosing retries.
augmentedS3Params.Body = await consumers.buffer(augmentedS3Params.Body);
}
uploadTask = this.s3.upload(augmentedS3Params);
if (showSpinner) {
uploadTask.on('httpUploadProgress', (max) => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,9 @@ const extractAPIModel = async (context: $TSContext, resource: $TSObject, framewo

fs.ensureDirSync(tempDir);

// This has been working fine before bumping node types to 18.x
// eslint-disable-next-line @typescript-eslint/ban-ts-comment
// @ts-ignore
sobolk marked this conversation as resolved.
Show resolved Hide resolved
const buff = Buffer.from(data.body);
fs.writeFileSync(`${tempDir}/${apiName}.zip`, buff);
await extract(`${tempDir}/${apiName}.zip`, { dir: tempDir });
Expand Down
4 changes: 4 additions & 0 deletions packages/amplify-provider-awscloudformation/src/zip-util.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,10 @@ export const downloadZip = async (s3: S3, tempDir: string, zipFileName: string,
log();
const objectResult = await s3.getFile({ Key: zipFileName }, envName);
fs.ensureDirSync(tempDir);

// This has been working fine before bumping node types to 18.x
// eslint-disable-next-line @typescript-eslint/ban-ts-comment
// @ts-ignore
const buff = Buffer.from(objectResult);
const tempFile = `${tempDir}/${zipFileName}`;
await fs.writeFile(tempFile, buff);
Expand Down
25 changes: 17 additions & 8 deletions yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -811,7 +811,7 @@ __metadata:
"@types/deep-diff": ^1.0.0
"@types/folder-hash": ^4.0.1
"@types/lodash.throttle": ^4.1.6
"@types/node": ^12.12.6
"@types/node": ^18.16.0
"@types/uuid": ^8.0.0
amplify-codegen: 4.7.3
archiver: ^5.3.0
Expand Down Expand Up @@ -12488,10 +12488,12 @@ __metadata:
languageName: node
linkType: hard

"@types/node@npm:*, @types/node@npm:^18.16.1":
version: 18.16.1
resolution: "@types/node@npm:18.16.1"
checksum: bd43d9e1df253955d73348ae9c8bc73f328ad3bd5481a134743142a04b0d1b74e39b90369aa0d361fd0b86a6a5c98035a226c1e1a4f67ba98e71b06734cc4f7d
"@types/node@npm:*, @types/node@npm:^18.16.0, @types/node@npm:^18.16.1":
version: 18.19.31
resolution: "@types/node@npm:18.19.31"
dependencies:
undici-types: ~5.26.4
checksum: bfebae8389220c0188492c82eaf328f4ba15e6e9b4abee33d6bf36d3b13f188c2f53eb695d427feb882fff09834f467405e2ed9be6aeb6ad4705509822d2ea08
languageName: node
linkType: hard

Expand All @@ -12503,9 +12505,9 @@ __metadata:
linkType: hard

"@types/node@npm:^16.9.2":
version: 16.11.14
resolution: "@types/node@npm:16.11.14"
checksum: 017037da8387c85ea92c4ecb06dfb1f511e398ec14504a9395bc92948d58840755066eba991e308b39cf1117648be20cd3c43d184f1ec1339bb60b836dd5e4e7
version: 16.18.96
resolution: "@types/node@npm:16.18.96"
checksum: 05ac1c80c8d075086863f7640fd3e75c3912c4ed067bb38bb8fd5377f4e64de7a81d5be3ceae5448dc90d9802a0c7b0d3376538759b91ea652d16cc6dc7de767
languageName: node
linkType: hard

Expand Down Expand Up @@ -31466,6 +31468,13 @@ node-pty@beta:
languageName: node
linkType: hard

"undici-types@npm:~5.26.4":
version: 5.26.5
resolution: "undici-types@npm:5.26.5"
checksum: bb673d7876c2d411b6eb6c560e0c571eef4a01c1c19925175d16e3a30c4c428181fb8d7ae802a261f283e4166a0ac435e2f505743aa9e45d893f9a3df017b501
languageName: node
linkType: hard

"unfetch@npm:^4.2.0":
version: 4.2.0
resolution: "unfetch@npm:4.2.0"
Expand Down
Loading