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.

Sign up for GitHub

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

Jump to bottom

resolved #407: Allocation failure when uploading larger files. #408

Merged
merged 3 commits into from
Mar 4, 2024
Merged

resolved #407: Allocation failure when uploading larger files. #408

merged 3 commits into from
Mar 4, 2024

Conversation

eric2788
Copy link
Contributor

@eric2788 eric2788 commented Feb 19, 2024 •
edited
Loading

  • there are some additional revises for optimization, feel free to remove them
  • there are some differences between this branch (not tested) and my current one (tested), so testing is required

@eric2788 eric2788 requested a review from ryand56 as a code owner February 19, 2024 04:17
@eric2788 eric2788 force-pushed the feature/multipart-upload branch from ae38bc8 to 88b29ae Compare February 20, 2024 01:56
ryand56
ryand56 reviewed Mar 2, 2024
View reviewed changes
Copy link
Owner

@ryand56 ryand56 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This does not seem to work on my local machine. I am test uploading a 1GB file and it is hanging up. Not sure what the issue is, and the retries aren't incrementing either.

ryand56
ryand56 requested changes Mar 2, 2024
View reviewed changes
Copy link
Owner

@ryand56 ryand56 left a comment

Choose a reason for hiding this comment

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

Some issues I found when testing the action. I have also gotten a 502 Bad Gateway error and no aborting of the upload.

src/index.ts
break;
} catch (err: any) {
retries++;
console.error(`R2 Error - ${err.message}, retrying: ${retries}/${config.maxTries}`, err);
Copy link
Owner

Choose a reason for hiding this comment

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

I'm getting a socket hang up error here

Copy link
Contributor Author

@eric2788 eric2788 Mar 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Is it without error details ?

Copy link
Owner

Choose a reason for hiding this comment

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

It is with error details

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can u show me the error details?

src/index.ts
await sleep(300);
}
}
if (retries >= config.maxTries) {
Copy link
Owner

Choose a reason for hiding this comment

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

This doesn't seem to run. I still have pending multipart uploads in my R2 bucket, and I've let the program run for a little bit.

Copy link
Contributor Author

@eric2788 eric2788 Mar 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I never met upload failed, so I didn't verify this, is there any way to reproduce the error ?

@eric2788
Copy link
Contributor Author

eric2788 commented Mar 2, 2024 •
edited
Loading

I will create a testing branch for this, so I may show you the test results ASAP.
But for reference, here's my current using one (not the PR one):
https://github.com/eric2788/bilibili-vup-stream-enhancer/actions/runs/8111204581/job/22170044936#step:12:431

ryand56
ryand56 reviewed Mar 2, 2024
View reviewed changes
Copy link
Owner

@ryand56 ryand56 left a comment

Choose a reason for hiding this comment

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

I will test the action again, making sure it wasn't a R2 issue.

@eric2788 eric2788 force-pushed the feature/multipart-upload branch from a54cf7c to 07ea5e3 Compare March 3, 2024 09:34
@eric2788
Copy link
Contributor Author

eric2788 commented Mar 3, 2024 •
edited
Loading

@ryand56 hey there, I just created a GitHub action for testing this branch: https://github.com/eric2788/r2-upload-action/actions/runs/8129299163/job/22216229621

original:

successfully uploaded:

broken format is normal because the original one already was

@eric2788
Copy link
Contributor Author

eric2788 commented Mar 3, 2024

I still got no idea how to test multipart upload fail 🤔 , the retries' mechanism still being unsure

ryand56
ryand56 approved these changes Mar 4, 2024
View reviewed changes
Copy link
Owner

@ryand56 ryand56 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I'm gonna say this works, since I uploaded one part until the socket hang up error appeared (maybe my internet). Error for reference:

R2 Info - Uploading tests/assets//1GB.bin (1.00 GB) to test-assets\1GB.bin R2 Success - Uploaded part 10.00 MB/1.00 GB of tests/assets//1GB.bin (1) R2 Error - socket hang up, retrying: 1/5 Error [TimeoutError]: socket hang up at connResetException (node:internal/errors:717:14) at TLSSocket.socketOnEnd (node:_http_client:526:23) at TLSSocket.emit (node:events:525:35) at endReadableNT (node:internal/streams/readable:1359:12) at processTicksAndRejections (node:internal/process/task_queues:82:21) { code: 'ECONNRESET', '$metadata': { attempts: 3, totalRetryDelay: 187 } }

Although, one question, does this overwrite files?

@ryand56 ryand56 merged commit 978fce6 into ryand56:develop Mar 4, 2024
@eric2788
Copy link
Contributor Author

eric2788 commented Mar 4, 2024 •
edited
Loading

I'm gonna say this works, since I uploaded one part until the socket hang up error appeared (maybe my internet). Error for reference:

R2 Info - Uploading tests/assets//1GB.bin (1.00 GB) to test-assets\1GB.bin R2 Success - Uploaded part 10.00 MB/1.00 GB of tests/assets//1GB.bin (1) R2 Error - socket hang up, retrying: 1/5 Error [TimeoutError]: socket hang up at connResetException (node:internal/errors:717:14) at TLSSocket.socketOnEnd (node:_http_client:526:23) at TLSSocket.emit (node:events:525:35) at endReadableNT (node:internal/streams/readable:1359:12) at processTicksAndRejections (node:internal/process/task_queues:82:21) { code: 'ECONNRESET', '$metadata': { attempts: 3, totalRetryDelay: 187 } }

Although, one question, does this overwrite files?

Only hit the complete upload request will overwrite files if the file key exist (just what's r2 does)

@eric2788
Copy link
Contributor Author

eric2788 commented Mar 4, 2024

Seems socket hang up error is unrecoverable because it immediately exits the process without continuing the statement 🤔

@eric2788 eric2788 deleted the feature/multipart-upload branch March 4, 2024 05:11
@ryand56
Copy link
Owner

ryand56 commented Mar 4, 2024

Seems socket hang up error is unrecoverable because it immediately exits the process without continuing the statement 🤔

Hmm, not sure why it's doing that, but I'll try to fix it in the new version

ryand56 pushed a commit that referenced this pull request Jul 31, 2024
@eric2788 @r2-action-bot
resolved #407: Allocation failure when uploading larger files. (#408)
0d2d7af 
* resolved #407: Allocation failure when uploading larger files.

* added concurrent multipart upload support (faster upload)

* [skip ci] Update mappings

---------

Co-authored-by: r2-action-bot[bot] <118486773+r2-action-bot[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ryand56 ryand56 ryand56 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@eric2788 @ryand56