Issue 360: Fix a bug in ResumableUpload when media size is unknown

Fix a bug in ResumableUpload when media size is unknown

[peleyal, 2013-07-22 01:02:58]

https://codereview.appspot.com/11347044/diff/3001/Src/GoogleApis.Tests/Apis/U...
File Src/GoogleApis.Tests/Apis/Upload/ResumableUploadTest.cs (left):

https://codereview.appspot.com/11347044/diff/3001/Src/GoogleApis.Tests/Apis/U...
Src/GoogleApis.Tests/Apis/Upload/ResumableUploadTest.cs:609: public void
TestChunkUpload_ServerUnavailable_KnownSize()
Add comment. This test covered the all 3 possibilities - server doesn't read
bytes on error, server read partial bytes on error, server reads all bytes but
somehow return 503

https://codereview.appspot.com/11347044/diff/3001/Src/GoogleApis.Tests/Apis/U...
File Src/GoogleApis.Tests/Apis/Upload/ResumableUploadTest.cs (right):

https://codereview.appspot.com/11347044/diff/3001/Src/GoogleApis.Tests/Apis/U...
Src/GoogleApis.Tests/Apis/Upload/ResumableUploadTest.cs:55:
ApplicationContext.RegisterLogger(new Google.Apis.Logging.Log4NetLogger());
leave it comment

https://codereview.appspot.com/11347044/diff/3001/Src/GoogleApis.Tests/Apis/U...
Src/GoogleApis.Tests/Apis/Upload/ResumableUploadTest.cs:307: /// <summary> Gets
or sets the amount of bytes there server reads on error. </summary>
there -> the
mount of bytes the server reads when error occurred. The default is 0, meaning
that on error the server won't read any bytes

https://codereview.appspot.com/11347044/diff/3001/Src/GoogleApis.Tests/Apis/U...
Src/GoogleApis.Tests/Apis/Upload/ResumableUploadTest.cs:646:
SubtestTestChunkUpload(true, 13, ServerError.ServerUnavailable, 50, 1);
add also tests for last chunk failed (e.g. chunk size = 300)

https://codereview.appspot.com/11347044/diff/3001/Src/GoogleApis.Tests/Apis/U...
Src/GoogleApis.Tests/Apis/Upload/ResumableUploadTest.cs:695: // we expect 4
calls: 1 initial request + 3 chunks (0-99, 100-199, 200-299), then the client
receive 4xx
then -> on the last chunk

https://codereview.appspot.com/11347044/diff/3001/Src/GoogleApis.Tests/Apis/U...
Src/GoogleApis.Tests/Apis/Upload/ResumableUploadTest.cs:701: int chunkSize =
100, int readBytesOnError = 0)
change the setter in ResumableUpload to check chunk size, and make the field
internal [VisibleToTestOnly] so we would be able to test it with "testable"
chunks

https://codereview.appspot.com/11347044/diff/3001/Src/GoogleApis/Apis/[Media]...
File Src/GoogleApis/Apis/[Media]/Upload/ResumableUpload.cs (left):

https://codereview.appspot.com/11347044/diff/3001/Src/GoogleApis/Apis/[Media]...
Src/GoogleApis/Apis/[Media]/Upload/ResumableUpload.cs:338: private async
Task<IUploadProgress> Upload(CancellationToken cancellationToken)
add TODO(peleyal): should we add a resume method?

https://codereview.appspot.com/11347044/diff/3001/Src/GoogleApis/Apis/[Media]...
File Src/GoogleApis/Apis/[Media]/Upload/ResumableUpload.cs (right):

https://codereview.appspot.com/11347044/diff/3001/Src/GoogleApis/Apis/[Media]...
Src/GoogleApis/Apis/[Media]/Upload/ResumableUpload.cs:167: /// <summary> Gets
and sets the amount of bytes the server had received so far. </summary>
or

https://codereview.appspot.com/11347044/diff/3001/Src/GoogleApis/Apis/[Media]...
Src/GoogleApis/Apis/[Media]/Upload/ResumableUpload.cs:170: /// <summary> Gets
and sets the amount of bytes the client had sent so far. </summary>
or

https://codereview.appspot.com/11347044/diff/3001/Src/GoogleApis/Apis/[Media]...
Src/GoogleApis/Apis/[Media]/Upload/ResumableUpload.cs:499: // that we need to
resend bytes from the last request
resend (and copy to the current request's stream)

https://codereview.appspot.com/11347044/diff/3001/Src/GoogleApis/Apis/[Media]...
Src/GoogleApis/Apis/[Media]/Upload/ResumableUpload.cs:565: // that we need to
change the position of the input stream
, otherwise we continue to send bytes form the current position

[peleyal, 2013-07-23 21:48:11]

https://codereview.appspot.com/11347044/diff/3001/Src/GoogleApis.Tests/Apis/U...
File Src/GoogleApis.Tests/Apis/Upload/ResumableUploadTest.cs (left):

https://codereview.appspot.com/11347044/diff/3001/Src/GoogleApis.Tests/Apis/U...
Src/GoogleApis.Tests/Apis/Upload/ResumableUploadTest.cs:609: public void
TestChunkUpload_ServerUnavailable_KnownSize()
On 2013/07/22 01:02:58, peleyal wrote:
> Add comment. This test covered the all 3 possibilities - server doesn't read
> bytes on error, server read partial bytes on error, server reads all bytes but
> somehow return 503 

Done.

https://codereview.appspot.com/11347044/diff/3001/Src/GoogleApis.Tests/Apis/U...
File Src/GoogleApis.Tests/Apis/Upload/ResumableUploadTest.cs (right):

https://codereview.appspot.com/11347044/diff/3001/Src/GoogleApis.Tests/Apis/U...
Src/GoogleApis.Tests/Apis/Upload/ResumableUploadTest.cs:55:
ApplicationContext.RegisterLogger(new Google.Apis.Logging.Log4NetLogger());
On 2013/07/22 01:02:58, peleyal wrote:
> leave it comment

Done.

https://codereview.appspot.com/11347044/diff/3001/Src/GoogleApis.Tests/Apis/U...
Src/GoogleApis.Tests/Apis/Upload/ResumableUploadTest.cs:307: /// <summary> Gets
or sets the amount of bytes there server reads on error. </summary>
On 2013/07/22 01:02:58, peleyal wrote:
> there -> the
> mount of bytes the server reads when error occurred. The default is 0, meaning
> that on error the server won't read any bytes

Done.

https://codereview.appspot.com/11347044/diff/3001/Src/GoogleApis.Tests/Apis/U...
Src/GoogleApis.Tests/Apis/Upload/ResumableUploadTest.cs:646:
SubtestTestChunkUpload(true, 13, ServerError.ServerUnavailable, 50, 1);
On 2013/07/22 01:02:58, peleyal wrote:
> add also tests for last chunk failed (e.g. chunk size = 300)

Done.

https://codereview.appspot.com/11347044/diff/3001/Src/GoogleApis.Tests/Apis/U...
Src/GoogleApis.Tests/Apis/Upload/ResumableUploadTest.cs:695: // we expect 4
calls: 1 initial request + 3 chunks (0-99, 100-199, 200-299), then the client
receive 4xx
On 2013/07/22 01:02:58, peleyal wrote:
> then -> on the last chunk

Done.

https://codereview.appspot.com/11347044/diff/3001/Src/GoogleApis.Tests/Apis/U...
Src/GoogleApis.Tests/Apis/Upload/ResumableUploadTest.cs:701: int chunkSize =
100, int readBytesOnError = 0)
On 2013/07/22 01:02:58, peleyal wrote:
> change the setter in ResumableUpload to check chunk size, and make the field
> internal [VisibleToTestOnly] so we would be able to test it with "testable"
> chunks

Done.

https://codereview.appspot.com/11347044/diff/3001/Src/GoogleApis/Apis/[Media]...
File Src/GoogleApis/Apis/[Media]/Upload/ResumableUpload.cs (right):

https://codereview.appspot.com/11347044/diff/3001/Src/GoogleApis/Apis/[Media]...
Src/GoogleApis/Apis/[Media]/Upload/ResumableUpload.cs:167: /// <summary> Gets
and sets the amount of bytes the server had received so far. </summary>
On 2013/07/22 01:02:58, peleyal wrote:
> or

Done.

https://codereview.appspot.com/11347044/diff/3001/Src/GoogleApis/Apis/[Media]...
Src/GoogleApis/Apis/[Media]/Upload/ResumableUpload.cs:170: /// <summary> Gets
and sets the amount of bytes the client had sent so far. </summary>
On 2013/07/22 01:02:58, peleyal wrote:
> or

Done.

https://codereview.appspot.com/11347044/diff/3001/Src/GoogleApis/Apis/[Media]...
Src/GoogleApis/Apis/[Media]/Upload/ResumableUpload.cs:499: // that we need to
resend bytes from the last request
On 2013/07/22 01:02:58, peleyal wrote:
> resend (and copy to the current request's stream)

Done.

https://codereview.appspot.com/11347044/diff/3001/Src/GoogleApis/Apis/[Media]...
Src/GoogleApis/Apis/[Media]/Upload/ResumableUpload.cs:565: // that we need to
change the position of the input stream
On 2013/07/22 01:02:58, peleyal wrote:
> , otherwise we continue to send bytes form the current position

Done.

[peleyal, 2013-07-23 22:05:10]

Hi Gus,
Like I promised attached is a fix to resumable media upload.
I fixed it in the Java client library and I was pretty sure that we are going to
have a similar bug in the .NET client library.

So I added more tests, and I'm happy with current implementation.

Thanks,
Eyal

[peleyal, 2013-08-02 21:39:01]

On 2013/07/23 22:05:10, peleyal wrote:
> Hi Gus,
> Like I promised attached is a fix to resumable media upload.
> I fixed it in the Java client library and I was pretty sure that we are going
to
> have a similar bug in the .NET client library.
> 
> So I added more tests, and I'm happy with current implementation.
> 
> Thanks,
> Eyal

Ping ... :)

[class, 2013-08-02 22:01:19]

A few documentation comments and a design consideration

https://codereview.appspot.com/11347044/diff/20006/Src/GoogleApis.Tests/Apis/...
File Src/GoogleApis.Tests/Apis/Upload/ResumableUploadTest.cs (right):

https://codereview.appspot.com/11347044/diff/20006/Src/GoogleApis.Tests/Apis/...
Src/GoogleApis.Tests/Apis/Upload/ResumableUploadTest.cs:52: public void SetUp()
Consider passing a boolean instead of commenting out

https://codereview.appspot.com/11347044/diff/20006/Src/GoogleApis.Tests/Apis/...
Src/GoogleApis.Tests/Apis/Upload/ResumableUploadTest.cs:308: /// Gets or sets
the amount of bytes the server reads when error occurred. The default value is
<c>0</c>,
Gets or sets the *number* of bytes

https://codereview.appspot.com/11347044/diff/20006/Src/GoogleApis.Tests/Apis/...
Src/GoogleApis.Tests/Apis/Upload/ResumableUploadTest.cs:654: /// An helper test
which tests that the client accepts 308 and 503 responses when uploading chunks.
This test
This should be "A helper test that tests..."

https://codereview.appspot.com/11347044/diff/20006/Src/GoogleApis.Tests/Apis/...
Src/GoogleApis.Tests/Apis/Upload/ResumableUploadTest.cs:1039: // less that the
minimum
Less than the minimum?

https://codereview.appspot.com/11347044/diff/20006/Src/GoogleApis/Apis/[Media...
File Src/GoogleApis/Apis/[Media]/Upload/ResumableUpload.cs (right):

https://codereview.appspot.com/11347044/diff/20006/Src/GoogleApis/Apis/[Media...
Src/GoogleApis/Apis/[Media]/Upload/ResumableUpload.cs:178: /// Gets or sets the
size of each chunk sent to the server.
I don't have too strong of an opinion on this but I think it would be acceptable
to remove the language pattern:

"Gets or sets the ..."

We could alternatively use something like:
"The size of each chunk sent to the server."

https://codereview.appspot.com/11347044/diff/20006/Src/GoogleApis/Apis/[Media...
Src/GoogleApis/Apis/[Media]/Upload/ResumableUpload.cs:510: // if the number of
bytes received by the sever isn't equal to the amount of bytes the client sent,
it
A revision:

 // if the number of bytes received by the server isn't equal to the amount of
bytes the client sent,
 // resend bytes and copy to the current request's stream from the last request

server was misspelled and the last part of the comment was made a little more
concise

[peleyal, 2013-08-05 15:15:29]

https://codereview.appspot.com/11347044/diff/20006/Src/GoogleApis.Tests/Apis/...
File Src/GoogleApis.Tests/Apis/Upload/ResumableUploadTest.cs (right):

https://codereview.appspot.com/11347044/diff/20006/Src/GoogleApis.Tests/Apis/...
Src/GoogleApis.Tests/Apis/Upload/ResumableUploadTest.cs:52: public void SetUp()
On 2013/08/02 22:01:20, class wrote:
> Consider passing a boolean instead of commenting out

Done.

https://codereview.appspot.com/11347044/diff/20006/Src/GoogleApis.Tests/Apis/...
Src/GoogleApis.Tests/Apis/Upload/ResumableUploadTest.cs:308: /// Gets or sets
the amount of bytes the server reads when error occurred. The default value is
<c>0</c>,
On 2013/08/02 22:01:20, class wrote:
> Gets or sets the *number* of bytes

Done.

https://codereview.appspot.com/11347044/diff/20006/Src/GoogleApis.Tests/Apis/...
Src/GoogleApis.Tests/Apis/Upload/ResumableUploadTest.cs:654: /// An helper test
which tests that the client accepts 308 and 503 responses when uploading chunks.
This test
On 2013/08/02 22:01:20, class wrote:
> This should be "A helper test that tests..."

Done.

https://codereview.appspot.com/11347044/diff/20006/Src/GoogleApis.Tests/Apis/...
Src/GoogleApis.Tests/Apis/Upload/ResumableUploadTest.cs:1039: // less that the
minimum
On 2013/08/02 22:01:20, class wrote:
> Less than the minimum?

Done.

https://codereview.appspot.com/11347044/diff/20006/Src/GoogleApis/Apis/[Media...
File Src/GoogleApis/Apis/[Media]/Upload/ResumableUpload.cs (right):

https://codereview.appspot.com/11347044/diff/20006/Src/GoogleApis/Apis/[Media...
Src/GoogleApis/Apis/[Media]/Upload/ResumableUpload.cs:178: /// Gets or sets the
size of each chunk sent to the server.
I prefer the 'gets or sets' just because it's the way the framework classes were
documented.
On 2013/08/02 22:01:20, class wrote:
> I don't have too strong of an opinion on this but I think it would be
acceptable
> to remove the language pattern:
> 
> "Gets or sets the ..."
> 
> We could alternatively use something like:
> "The size of each chunk sent to the server."

https://codereview.appspot.com/11347044/diff/20006/Src/GoogleApis/Apis/[Media...
Src/GoogleApis/Apis/[Media]/Upload/ResumableUpload.cs:510: // if the number of
bytes received by the sever isn't equal to the amount of bytes the client sent,
it
On 2013/08/02 22:01:20, class wrote:
> A revision:
> 
>  // if the number of bytes received by the server isn't equal to the amount of
> bytes the client sent,
>  // resend bytes and copy to the current request's stream from the last
request
> 
> server was misspelled and the last part of the comment was made a little more
> concise

Done.

[class, 2013-08-08 17:32:31]

There is still one typo I missed the first time, LGTM after renaming.

https://codereview.appspot.com/11347044/diff/20006/Src/GoogleApis.Tests/Apis/...
File Src/GoogleApis.Tests/Apis/Upload/ResumableUploadTest.cs (right):

https://codereview.appspot.com/11347044/diff/20006/Src/GoogleApis.Tests/Apis/...
Src/GoogleApis.Tests/Apis/Upload/ResumableUploadTest.cs:424: if (bytesRecieved
!= len)
Minor spelling nit: bytesReceieved

https://codereview.appspot.com/11347044/diff/38001/Src/GoogleApis.Tests/Apis/...
File Src/GoogleApis.Tests/Apis/Upload/ResumableUploadTest.cs (right):

https://codereview.appspot.com/11347044/diff/38001/Src/GoogleApis.Tests/Apis/...
Src/GoogleApis.Tests/Apis/Upload/ResumableUploadTest.cs:328: private int
bytesRecieved = 0;
Missed this the first time, this should be bytesReceived, minor typo

https://codereview.appspot.com/11347044/diff/38001/Src/GoogleApis.Tests/Apis/...
Src/GoogleApis.Tests/Apis/Upload/ResumableUploadTest.cs:445: if (bytesRecieved
!= len)
(as above) refactor to bytesReceived

[peleyal, 2013-08-08 20:45:01]

My bad.
Waiting for your approval.

Thanks,
Eyal

https://codereview.appspot.com/11347044/diff/20006/Src/GoogleApis.Tests/Apis/...
File Src/GoogleApis.Tests/Apis/Upload/ResumableUploadTest.cs (right):

https://codereview.appspot.com/11347044/diff/20006/Src/GoogleApis.Tests/Apis/...
Src/GoogleApis.Tests/Apis/Upload/ResumableUploadTest.cs:424: if (bytesRecieved
!= len)
On 2013/08/08 17:32:31, class wrote:
> Minor spelling nit: bytesReceieved

Done.

https://codereview.appspot.com/11347044/diff/38001/Src/GoogleApis.Tests/Apis/...
File Src/GoogleApis.Tests/Apis/Upload/ResumableUploadTest.cs (right):

https://codereview.appspot.com/11347044/diff/38001/Src/GoogleApis.Tests/Apis/...
Src/GoogleApis.Tests/Apis/Upload/ResumableUploadTest.cs:328: private int
bytesRecieved = 0;
On 2013/08/08 17:32:31, class wrote:
> Missed this the first time, this should be bytesReceived, minor typo

Done.

https://codereview.appspot.com/11347044/diff/38001/Src/GoogleApis.Tests/Apis/...
Src/GoogleApis.Tests/Apis/Upload/ResumableUploadTest.cs:445: if (bytesRecieved
!= len)
On 2013/08/08 17:32:31, class wrote:
> (as above) refactor to bytesReceived

Done.

[class, 2013-08-08 21:44:28]

LGTM