[WIP] Make downloads less bad (refactoring & recognizing stale connections) #1790
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
Conversation
There was a problem hiding this comment.
Thanks @jtc0de
I think this is looking good in general and I would be happy to merge it soon. I like that you moved to a dedicated module, so we isolate things furthermore. It will make it easier to further unit test it and refactor in later PRs.
However I still have a few concerns, listed below:
- I would completely remove Python2 branching now that we dropped support for it.
- Our HTTP download still feels too complex to me for what we're doing. But it was already prior your PR. Also a lot of it can be solved by point 1 above.
- Since you made this PR we added some unit testing to the download part, see Unit tests Recipe.download_file() partly #1952. Can you rebase and make sure tests still turn green.
Thanks!
| except (socket.timeout, http.client.IncompleteRead): | ||
| raise OSError("reading timed out") | ||
| else: | ||
| # no http.client module in python 2 |
There was a problem hiding this comment.
We don't support Python2 anymore, can we kill all the branching of this PR to make it more simple?
The general aim of this pull request is to make downloads less bad, both in code and in behavior. I did some refactoring and moving around to clean things up, and I revamped e.g. the http/wget-alike download function to recognize if no new data arrives in 25 seconds and treat that as an error (previously, there was no timeout for stale connections, or it was an insanely long duration)
Still WIP because I want to add some mock testing