-
Notifications
You must be signed in to change notification settings - Fork 344
Added the ability to upload files larger than 4MB #2180
Conversation
} | ||
index += length; | ||
bytesLeft -= length; | ||
if (bytesLeft == 0) break; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: why not use this as the while loop condition, especially since we do this check at the end of the loop anyway?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will do
{ | ||
FilePath = filePath; | ||
FileContent = fileContent; | ||
if (offset < 0) throw new ArgumentOutOfRangeException(nameof(offset)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to add checks for offset greater than _length?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
offset can be greater, equal, or less than length. It cannot be greater than fileContent.Length - _length. I will add the check.
writer.WriteHeader("x-ms-date", Time, 'R'); | ||
// TODO (pri 3): this allocation should be eliminated | ||
writer.WriteHeader("x-ms-range", $"bytes=0-{size - 1}"); | ||
|
||
var start = arguments._offset; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: use of var
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will fix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Otherwise, looks good! Can't wait to try it :)
{ | ||
Memory<byte> buffer = writer.GetMemory(); | ||
if(buffer.Length > bytesToWrite) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: formatting, add space
@@ -131,14 +138,21 @@ class Writer : StorageRequestWriter<PutRangeRequest> | |||
public override Http.Method Verb => Http.Method.Put; | |||
|
|||
protected override async Task WriteBody(PipeWriter writer, PutRangeRequest arguments) | |||
=> await writer.WriteAsync(arguments.FileContent); | |||
{ | |||
var stream = arguments._fileContent; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: use of var
{ | ||
FilePath = filePath; | ||
FileContent = fileContent; | ||
if (offset < 0 || offset > fileContent.Length - length) throw new ArgumentOutOfRangeException(nameof(offset)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. What about the case when length > fileContent.Length
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AORE will be thrown. fileContent.Length - length will be negative and will be < offset (which has to be positive at the time of this check)
b611deb
to
474d53f
Compare
@@ -212,7 +212,7 @@ static void TransferStorageFileToDirectory(ReadOnlySpan<char> storageFile, ReadO | |||
} | |||
index += length; | |||
bytesLeft -= length; | |||
if (bytesLeft == 0) break; | |||
Debug.Assert(bytesLeft >= 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this Debug.Assert is necessary, given the loop condition is already stronger. Maybe a better option would be to add a Debug.Assert after the loop exits, if we know that bytesLeft should never be negative and if that signals a coding bug:
Debug.Assert(bytesLeft == 0);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The assert ensures bytesLeft is no negative
cc: @ahsonkhan