-
Notifications
You must be signed in to change notification settings - Fork 219
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
Skip files which were uploaded completely, but the server did not return any response #372
base: master
Are you sure you want to change the base?
Conversation
5225be8
to
2469d6d
Compare
It seems flake8 fails because the |
2469d6d
to
b33c908
Compare
b33c908
to
148e6f5
Compare
148e6f5
to
4f11916
Compare
4f11916
to
c60e581
Compare
c60e581
to
875d9b3
Compare
875d9b3
to
a124614
Compare
Do you want to have another look at this PR? I have been using this patch for a year now and it improves uploading of large batches significantly for me. See the comments in the issue linked above for some people who suffer the same issues. The code doesn't do anything complex. Just some fine-grained exception handling to ignore some errors (raised incorrectly) by the server. |
I'm not certain I like this as the default, but I think it could be handy as an option. I think it's important to raise an exception (or have a non-zero exit status code), if there's a chance the upload wasn't successful. What are your thoughts on that @Dobatymo? |
@jjjake I think raising an exception is not the way to go. This error is extremely common for me when uploading batches of large files. Sometimes every file encounters this error, but I have yet to find a file where this error occured and it was not actually fully uploaded. That's why I think retrying is not good also. Of course I could return another exit code if this error was encountered for any of the files in the batch. |
a124614
to
95508c9
Compare
95508c9
to
b0965c0
Compare
b0965c0
to
51625eb
Compare
What's the status of this? |
@soredake The patch was working well 2-3 years ago. I haven't tested recently. New versions of requests or urllib3 might have changed. |
In some cases the server closes the connection after the file was fully uploaded without sending a response. See my comment here #341 (comment)
This doesn't fix the issue of OP, but makes batch uploading of a large number of files easier. I am only handling the
RemoteDisconnected
exception, which is only raised when the client tries to read the http status line after sending the request.In my testing the files were always uploaded completely in that case. So rerunning the batch upload with skipping with checksums enabled, it would skip these 'failed' failes correctly. So in case this error is encountered, it will show a informative message and continue with the next file instead of raising the exception and stopping the batch upload.