Fix infinite loop if Reader returns io.ErrUnexpectedEOF #620
Conversation
| @@ -508,6 +508,7 @@ func (u *multiuploader) upload(firstBuf io.ReadSeeker) (*UploadOutput, error) { | |||
|
|
|||
| // Read and queue the rest of the parts | |||
| for u.geterr() == nil { | |||
| num++ | |||
There was a problem hiding this comment.
What happens is that after the change it does one less iteration of the loop exiting at the first io.ErrUnexpectedEOF instead of iterating again and exiting at the io.EOF.
I moved it to make the unit tests pass!
There was a problem hiding this comment.
Wouldn't this always cause one less iteration? It may be best to update the unit tests.
There was a problem hiding this comment.
My reading of the code is that it is correct now.
num starts on 1, because one part gets sent before the loop starts.
Say MaxUploadParts is 2.
First time around the loop num gets incremented to 2 before the check against MaxUploadParts.
The num > MaxUploadParts check succeeds and we send that part (the second).
Second time around the loop num gets incremented to 3 - the check fails and the loop is aborted before sending the 3rd part.
The test that fails without this change is TestUploadOrderMultiBufferedReaderExceedTotalParts. Looking at that code I think that is correct.
It worked before because the loop did one extra iteration it shouldn't have done.
I could be wrong though so a second opinion would be good!
There was a problem hiding this comment.
The previous change would increment to 3 and then break on an io.EOF since there are no more parts. In this case it would start at 1, increment to 2 and upload the next part with no errors. Finally, increment to three and set the err to the excess error, but we want to break on an io.EOF and have nothing set to the error. Does that sound right? Let me know if I am missing something
There was a problem hiding this comment.
I think before my change it sent one part too many
- send first part outside loop
- num = 1
- if num > MaxUploadParts -> no
- num++ (2)
- send second part
- if num > MaxUploadParts -> no
- num++ (3)
- send third part
- if num > MaxUploadParts -> yes, break
There was a problem hiding this comment.
- send first part outside loop
- num = 1
- if num > MaxUploadParts -> no
- num++ (2)
- send second part
- if num > MaxUploadParts -> no
- num++ (3)
- nextReader() return err
- err is io.EOF
- break
Lemme see what that unit test is doing
There was a problem hiding this comment.
Okay, nvm. Drew it out. I see what you are saying. Can you add one more unit test to test for EOF?
|
Hello @ncw, thank you for taking the time to put this PR together. I am going to reach out to S3 and see when an |
|
I haven't observed this directly, but a user of my rclone program reported the problem and we debugged it together. See rclone/rclone#415 I think this error is very unlikely to occur normally, however my user could make it happen reliably with their ceph cluster. According to the thread I linked ErrUnexpectedEOF can happen when using https. I've tested the patch as has my user with the problem and it passes the unit tests, though travis seems to be broken at the moment (probably because of this https://groups.google.com/forum/#!topic/golang-announce/JMs4_CGbXWk) |
|
@ncw - Okay, I'll forward this to S3. |
|
@ncw - Fixed travis. If you could resync with master, that'd be great! |
|
@ncw - Okay, your changes make sense. Once you add the other unit test and sync with master I can merge it in |
Apparently some HTTP transactions can return io.ErrUnexpectedEOF, eg https://groups.google.com/forum/#!topic/golang-nuts/2wDPB42-rZ0 Before this change, this would result in S3Manager sending a lot of 0 sized parts, eventually causing the error "TotalPartsExceeded: exceeded total allowed configured MaxUploadParts".
|
I've updated and rebased the PR - let me know what you think! Thanks Nick |
|
@ncw - Awesome! Thanks for the update! The changes look good. |
ghost
mentioned this pull request
Apparently some HTTP transactions can return io.ErrUnexpectedEOF, eg
https://groups.google.com/forum/#!topic/golang-nuts/2wDPB42-rZ0
Before this change, this would result in S3Manager sending a lot of 0
sized parts, eventually causing the error "TotalPartsExceeded:
exceeded total allowed configured MaxUploadParts".