Skip to content

Conversation

@jsalles
Copy link
Contributor

@jsalles jsalles commented Apr 30, 2024

Problem

There are currently 2 problems in featureDev when doing collection of repository files:

  • Trying to decode binary files might crash the extension host, and
  • too large repos might make the extension run out of available memory.

Solution

This CR fixes both problems by:

  • Adding a maximum allowed repo size of 200mb for collection
  • Changing the behavior of the TextDecoder to replace the unsupported characters instead of throwing TypeError

Users who try to use featureDev on a codebase larger than 200mb will see a message asking for a different workspace to be selected.

License

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@jsalles jsalles force-pushed the collect-files-crash branch from 7a8d0ab to 3eb3438 Compare April 30, 2024 11:00
@jsalles jsalles force-pushed the collect-files-crash branch from 3eb3438 to 858e55a Compare April 30, 2024 11:51
@jpinkney-aws
Copy link
Contributor

Just one more thing, are you able to add a changelog? I'm sure theres some users that would like to know this is fixed 😄

@jsalles jsalles force-pushed the collect-files-crash branch from 858e55a to 0d1ceea Compare April 30, 2024 13:39
@jsalles jsalles requested a review from a team as a code owner April 30, 2024 13:39
@jsalles
Copy link
Contributor Author

jsalles commented Apr 30, 2024

Just one more thing, are you able to add a changelog? I'm sure theres some users that would like to know this is fixed 😄

Yes, definitely! Just added a changelog with the bugfix :)

) {
try {
const files = await collectFiles(repoRootPaths, workspaceFolders, true)
const zip = new AdmZip()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the current approach used to zip files does not "stream" the zip creation so will also run into memory constraints.

please look at #4769 , can you rewrite this feature on top of that (after it lands)?

Copy link
Contributor

@grimalk grimalk Apr 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the ETA of this being ready? Presume we want both these in the same wave of the toolkit?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I'm happy to change our implementation to use the stream after it's merged.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the ETA of this being ready?

The zip streaming won't be ready today, but hopefully next week. @ctlai95

@jsalles jsalles force-pushed the collect-files-crash branch from 0d1ceea to 37b71b0 Compare April 30, 2024 15:12
@jsalles jsalles force-pushed the collect-files-crash branch from 37b71b0 to 21428f5 Compare April 30, 2024 15:54
@justinmk3 justinmk3 merged commit fa680b5 into aws:master Apr 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants