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

Change image upload chunk size from 512 * 3/4 KiB to 512 KiB #1650

Merged
merged 1 commit into from
Jul 11, 2023

Conversation

david-crespo
Copy link
Collaborator

@david-crespo david-crespo commented Jul 11, 2023

@luqmana found while working on oxidecomputer/omicron#3559 and #1469 that changing the chunk size to match what the CLI does makes it impossible to reproduce the image upload hang. Obviously we will want to figure out why it didn't work with the other chunk size (it should work with any) but in the meantime we can prevent the issue from happening.

This made me realize the old chunk size was based on a misunderstanding on my part of how the limit was being applied. I had it backwards. If base64 makes the data 1/3 bigger, and we are chopping up base64ed data, we can actually send a string of length 3/4 * MAX (not MAX * 3/4) because that will decode to a byte array of length MAX. Here is the bit in crucible where the length of a Vec<u8> is checked against MAX_CHUNK_SIZE.

On top of that, I was doubly confused — I forgot the File.prototype.slice(start, end) API takes byte offsets as start and end, even though we are able to read the contents of the slice more or less directly as a base64 string. So 512 KiB is in fact the correct max, matching the CLI and crucible.

@david-crespo david-crespo merged commit df8b509 into main Jul 11, 2023
@david-crespo david-crespo deleted the normal-chunk-size branch July 11, 2023 22:06
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.

1 participant