Improve AWS upload mechanism and allow to override registry timeout settings #516
Conversation
| @@ -60,7 +60,7 @@ | |||
| "dependencies": { | |||
| "accept-language-parser": "1.4.0", | |||
| "async": "2.4.1", | |||
| "aws-sdk": "2.62.0", | |||
| "aws-sdk": "2.67.0", | |||
There was a problem hiding this comment.
A bug was preventing the upload() S3 function to properly upload all the chunks. Latest version fixes 👌
| @@ -50,6 +48,7 @@ module.exports = function(input) { | |||
| .map(s => s.toLowerCase()); | |||
|
|
|||
| options.port = process.env.PORT || options.port; | |||
| options.timeout = options.timeout || 1000 * 60 * 2; | |||
There was a problem hiding this comment.
This is the timeout for the http server, after which a request gets terminated with a "ECONNRESET".
When trying to upload a big file, I changed this 10m and worked fine. I opted for a 2m default (which is the node default) but left customisable by the user.
|
|
||
| callback(null, 'ok'); | ||
| } | ||
| callback |
There was a problem hiding this comment.
async.each actually fires the error as soon as it gets the first. This means that the "compact" is not necessary as the error is just 1. When testing locally, I was seeing a error with a [Object object] result here prior of this fix in case of S3 issues.
|
|
||
| getClient().putObject(obj, callback); | ||
| const upload = getClient().upload(obj); | ||
| upload.send(callback); |
There was a problem hiding this comment.
For large files, the recommended approach is to use upload over putObject. Upload automatically processes the req into multiple chunks and its Body property accepts both string (making this change retrocompatible) and streams (see change in the putFile method).
| }); | ||
| try { | ||
| const stream = fs.createReadStream(filePath); | ||
| putFileContent(stream, fileName, isPrivate, callback); |
There was a problem hiding this comment.
Here we just create a stream reader instead of reading the whole content. For big files, this is actually necessary for not blocking the event loop with large GCs operations.
| _.each(defaults, (value, property) => { | ||
| it(`should set ${property} to ${JSON.stringify(value)}`, () => { | ||
| expect(sanitise(options)[property]).to.eql(value); | ||
| }); |
There was a problem hiding this comment.
I wanted to test timeout returning the default, but I just ended up testing each property of the snapshot. When migrating to jest, this will be so much prettier ❤️
| 'header1', | ||
| 'header-two', | ||
| 'header3' | ||
| ]); |
There was a problem hiding this comment.
This seems to be a prettier initiative.
| putFileContent(fileContent, fileName, isPrivate, callback); | ||
| }); | ||
| try { | ||
| const stream = fs.createReadStream(filePath); |
There was a problem hiding this comment.
This is now inside a try/catch because a exception will be fired in case of file not found.
matteofigus
changed the title
Fixes #514
This Pull Request aims to address some necessary optimisations to the overall publish strategy, specifically with dealing with big components (including many and/or big files) and possibly slow connections.
In this perspective, here
putObjecttouploadmethod. This allows uploads into multiple chunks and is necessary for files that reach big dimensionsSee inline comments for details about the implementation.