Skip to content

Conversation

@passchaos
Copy link
Contributor

No description provided.

@sfackler
Copy link
Contributor

What is this protecting against?

@sfackler
Copy link
Contributor

Oh, nevermind. I see.

It seems really bad that this is reading 16 bytes at a time. That buffer should be like 1000 times larger than it is.

@jolhoeft
Copy link
Contributor

The small buffer is deliberate since the test file is so small. There is a comment about that, but further up the file.

@passchaos
Copy link
Contributor Author

Yeah, the buffer is too small to get a fast download speed, but more importly, the bound is necessary to get the consistent file.

@jolhoeft
Copy link
Contributor

Yes, the buffer is too small for speed, this is just to illustrate how to do it. Real world applications will need to use a larger buffer, which is mentioned in a comment, although perhaps that should be next to the buffer allocation so it doesn't get missed.

The bounds are confusing me. It is specifying a range that is simply all the data that was just read into the buffer. From my reading of the Read trait's docs, implementers are just supposed to overwrite whatever is there, so specifying a full range should be unnecessary. I tested various buffer sizes, and never saw a problem with partial buffers.

@passchaos
Copy link
Contributor Author

In most cases, the size of the file is not a multiple of that of the 16, so, the last time to read a file to the buffer will not fill the buffer, without boundary, send files will be compared to the original file more than a few bytes.

@jolhoeft
Copy link
Contributor

Ok, I tested, and you are right, we are getting trailing data, but the browser is ignoring them. I was confusing it with a Vec.

@seanmonstar seanmonstar merged commit 73511ac into hyperium:master Jan 16, 2018
@seanmonstar
Copy link
Member

Thanks!

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.

4 participants