Skip to content
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

resolve read_timeout #248

Merged
merged 16 commits into from
Jun 20, 2017
Merged

resolve read_timeout #248

merged 16 commits into from
Jun 20, 2017

Conversation

thehesiod
Copy link
Collaborator

@thehesiod thehesiod commented Jun 9, 2017

@thehesiod thehesiod requested a review from jettify June 9, 2017 01:23
@thehesiod
Copy link
Collaborator Author

hey Jettify, let me know what you think. All this because aiohttp dev didn't want to fix a core issue in their impl :(

@codecov-io
Copy link

codecov-io commented Jun 9, 2017

Codecov Report

Merging #248 into master will increase coverage by 1.08%.
The diff coverage is 97.05%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #248      +/-   ##
==========================================
+ Coverage   86.28%   87.36%   +1.08%     
==========================================
  Files           8        8              
  Lines         452      467      +15     
==========================================
+ Hits          390      408      +18     
+ Misses         62       59       -3
Impacted Files Coverage Δ
aiobotocore/endpoint.py 90.44% <97.05%> (+3.12%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f722deb...ad745ef. Read the comment docs.

@jettify
Copy link
Member

jettify commented Jun 17, 2017

I think aiohttp will do right thing eventually, we just need to make sure we can remove it easily and be very careful for each aiohttp release.

By the way next aiohttp will have new multi dict that preserves original headers

@thehesiod
Copy link
Collaborator Author

We shall see since I can't drive that anymore given his response. Should I merge this PR or would you like me change something? Thanks!

@jettify
Copy link
Member

jettify commented Jun 19, 2017

change looks good, it uses a bit internal API but I do not see better way. We can merge, but lets do not close #245 at some point we need revert this PR once aiohttp ready

@thehesiod thehesiod changed the title first attempt at resolve read_timeout resolve read_timeout Jun 19, 2017
@thehesiod
Copy link
Collaborator Author

@jettify take a look at the last commit :) Will make our lives easier

@thehesiod thehesiod merged commit 734ba11 into master Jun 20, 2017
@thehesiod thehesiod deleted the thehesiod/read-timeout-fix branch June 20, 2017 00:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Tracking] Config read_timeout not behaving correctly
3 participants