feat: bugfix for artifacts upload #749
Conversation
| //use fs.createReadStream() to handle the file in smaller, manageable chunks rather than loading the entire file | ||
| // into memory. This avoids hitting the 2 GiB limit that occurs when using fs.readFile(), | ||
| // as it loads the entire file into memory. | ||
| static async getSha256(fileName: string): Promise<string> { |
| @@ -199,10 +212,17 @@ export class ArtifactManager { | |||
| }) | |||
| } | |||
|
|
|||
| calculateMD5Sync(filePath: string): string { | |||
| async calculateMD5Sync(filePath: string): Promise<string> { | |||
There was a problem hiding this comment.
- Rename to
calculateMD5Async - Do we need to
awaitany calls to this function now that it is async?
There was a problem hiding this comment.
This is the only place
| const response = await got.put(resp.uploadUrl, { | ||
| body: fs.readFileSync(fileName), | ||
| body: fileStream, |
There was a problem hiding this comment.
Does this work? Seems like got has a separate API for streams
There was a problem hiding this comment.
it's working for stream.
There was a problem hiding this comment.
got is not browser-compatible. In the vscode extension we settled on cross-fetch, and are using a wrapper so that modules can avoid direct dependencies on whatever package we use: https://github.com/aws/aws-toolkit-vscode/blob/2200b1544d0c0c3e250b8ae2dc16d5f3e87c4270/packages/core/src/shared/request.ts#L6
|
CI build is failing with message |
pranav-firake
changed the title
| //use fs.createReadStream() to handle the file in smaller, manageable chunks rather than loading the entire file | ||
| // into memory. This avoids hitting the 2 GiB limit that occurs when using fs.readFile(), | ||
| // as it loads the entire file into memory. | ||
| static async getSha256Async(fileName: string): Promise<string> { |
There was a problem hiding this comment.
As a small optimization, is it possible/sensible to calculate the sha256 and md5 of files at the same time and store these details in the artifact manager, that way we can get rid of some extra overhead by reducing the number of times fsCreateReadStream gets called on a single file.
If the timing of these hash calculations matters and if such an optimization is not feasible, feel free to ignore this comment
pranav-firake
changed the title
This reverts commit 71c0a19.
Problem
Node limit for 2 GiB for fs.readFile . ‘File size is greater than 2 GiB’ is common error in node with fs.readFile.
Reference nodejs/node#55864
We are using fs.readFile in 3 places in language server code.
Solution
use fs.createReadStream instead.
License
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.