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

Files are loaded into memory instead of streamed #52

Closed
enumag opened this issue Nov 5, 2023 · 8 comments · Fixed by #55
Closed

Files are loaded into memory instead of streamed #52

enumag opened this issue Nov 5, 2023 · 8 comments · Fixed by #55
Assignees
Labels
enhancement New feature or request Stale

Comments

@enumag
Copy link

enumag commented Nov 5, 2023

Hi, I checked your source code and found this line:

https://github.com/xresloader/upload-to-github-release/blob/8e0ffc2bf70a36035a8b5479d0903041cb11423f/src/index.ts#L721C37-L721C49

Releases can contain several large files so this way the runner can easily run out of memory and crash.

I found this while looking for alternatives when I discovered the same issue in another similar tool.

@owent owent self-assigned this Nov 7, 2023
@owent
Copy link
Member

owent commented Nov 7, 2023

Good point and thanks.

@enumag
Copy link
Author

enumag commented Nov 7, 2023

I'm no JS expert but probably you need to use something like this: https://www.geeksforgeeks.org/node-js-fs-createreadstream-method/

@owent
Copy link
Member

owent commented Nov 7, 2023

I'm no JS expert but probably you need to use something like this: https://www.geeksforgeeks.org/node-js-fs-createreadstream-method/

GitHub's APIs do not accept read stream as data parameter. I will try to use @file_path to see if it will use stream internally.

@enumag
Copy link
Author

enumag commented Nov 7, 2023

Ouch... that would be rather nasty if Octokit itself requires the data to be loaded in memory. 😳 I hope it's not the case.

@owent owent mentioned this issue Nov 7, 2023
Merged
@owent owent closed this as completed in #55 Nov 7, 2023
@owent owent reopened this Nov 7, 2023
@owent
Copy link
Member

owent commented Nov 7, 2023

I have raise #55 to try to use @file_path to upload file. But I have no repo which can be used to test a large asset file.
Could you please try v1 branch or main branch to see if it work?

@enumag
Copy link
Author

enumag commented Nov 7, 2023

It doesn't work. The action succeeded and seemingly did actually upload the files however the files are just a few bytes large and contain the path to the file in the worker rather than the correct content.

@owent owent added enhancement New feature or request do-not-stale labels Nov 8, 2023
@owent
Copy link
Member

owent commented Nov 8, 2023

Thanks, I will try to find another way to fix this problem later.

@owent owent removed the do-not-stale label Nov 8, 2023
Copy link

github-actions bot commented Feb 7, 2024

This issue was marked as stale due to lack of activity.

@github-actions github-actions bot added the Stale label Feb 7, 2024
@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Feb 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Stale
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants