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

[Major Bug] Incorrect chunk size #1988

Closed
AJHoeh opened this issue Jun 2, 2021 · 3 comments
Closed

[Major Bug] Incorrect chunk size #1988

AJHoeh opened this issue Jun 2, 2021 · 3 comments

Comments

@AJHoeh
Copy link

AJHoeh commented Jun 2, 2021

Describe the bug
When using chunked upload the first chunk has the correct size, the following have not. Their size seems to be filesize - number_already_sent_chunks * chunksize.

Didnt notice at first because it makes no difference for files <= 2*chunksize. Depending on how you merge the chunks there wont even be a difference for bigger files (beside very slow upload), at worst the uploaded file will differ in size: The first 2 chunks sum to the complete file with alot of unnoticed garbage bytes added. Files can still be opened, pdfs can be read and so on, to spot the difference file sizes have to be compared.

Probably related to #1904 and maybe even causes it.

To Reproduce
Steps to reproduce the behavior:

  1. Upload file with e.g. 10*chunksize
  2. Inspect every incoming request and the size of the recieved file

Expected behavior
Chunked upload will send requests with chunks of size chunksize (last chunk could be smaller of course)

Screenshots
Uploaded File:
uploaded_file
Chunks sent (first 18/20):
chunks

Note: If I inspect the requests in a browser the chunksizes are the same - this is just an easier way to give an impression of the size decay without taking 18 screenshots.

Additional context
We stumbled across this in production, big uploads are rare but this is a huge problem.

EDIT: console.log()'ed end after it is set here

let end = Math.min(
and the results show the error:
end
Problem is that the options are strings so without explicit typecasting start + this.options.chunkSize will e.g. result in 05000000, 50000005000000 and so on if chunksize is set to 5000000. Therefore typecasting at least the latter variable is necessary but I am not familiar with the code so please check other areas where this could be relevant too.

AJHoeh added a commit to AJHoeh/dropzone that referenced this issue Jun 2, 2021
@AJHoeh AJHoeh changed the title Incorrect chunk size [Major Bug] Incorrect chunk size Jun 8, 2021
@gplwhite
Copy link
Contributor

I'm a bit confused when you say

the options are strings

The default options have the following value for the chunkSize option
chunkSize: 2000000,

Javascript should be interpreting this as a number - not a string.

Are you sure you're not overriding the chunkSize option with a string value when customizing the options?

@AJHoeh
Copy link
Author

AJHoeh commented Sep 7, 2021

I'm a bit confused when you say

the options are strings

The default options have the following value for the chunkSize option
chunkSize: 2000000,

Javascript should be interpreting this as a number - not a string.

Are you sure you're not overriding the chunkSize option with a string value when customizing the options?

Hey, you are right and I am aware of that - but this happens easily e.g. if you load the values from config files. We all know JS won't do the handling of wrong parameter types for us and will do implicit coercion if it must, so the burden on checking the input is on us devs. Sure, users being informerd and providing the right input format is nice and all but I strongly believe that you should not rely on that and a function taking user input should always do type checks and/or do explicit coercian, espescially in cases where the script will still work without any warnings or errors and silently corrupt the user input.

@enyo
Copy link
Collaborator

enyo commented Oct 25, 2021

@AJHoeh so am I right to assume that the problem is actually that you provided a string instead of a number?

I can add a safety check to make sure this can't happen.

Thanks @gplwhite for helping out with this!

I'm closing this for now, unless there is something else going on that I didn't get.

@enyo enyo closed this as completed Oct 25, 2021
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

No branches or pull requests

3 participants