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.

Sign up for GitHub

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

Jump to bottom

Fix connection for multiple dns hosts #2447

Merged
merged 5 commits into from
Nov 1, 2017
Merged

Fix connection for multiple dns hosts #2447

merged 5 commits into from
Nov 1, 2017

Conversation

hellysmile
Copy link
Member

@hellysmile hellysmile commented Oct 30, 2017 •
edited
Loading

What do these changes do?

Fix for aiohttp.connector.TCPConnector._create_direct_connection tries to use all of dns hosts from dns response to connect one by one

Related issue number

#2424

Checklist

  • I think the code is well written
  • Unit tests for the changes exist
  • Documentation reflects the changes
  • If you provide code modification, please add yourself to CONTRIBUTORS.txt
    • The format is <Name> <Surname>.
    • Please keep alphabetical order, the file is sorted by names.
  • Add a new news fragment into the CHANGES folder
    • name it <issue_id>.<type> for example (588.bug)
    • if you don't have an issue_id change it to the pr id after creating the pr
    • ensure type is one of the following:
      • .feature: Signifying a new feature.
      • .bugfix: Signifying a bug fix.
      • .doc: Signifying a documentation improvement.
      • .removal: Signifying a deprecation or removal of public API.
      • .misc: A ticket has been closed, but it is not of interest to users.
    • Make sure to use full sentences with correct case and punctuation, for example: "Fix issue with non-ascii contents in doctest text files."

@codecov-io
Copy link

codecov-io commented Oct 30, 2017 •
edited
Loading

Codecov Report

Merging #2447 into 2.3 will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##              2.3    #2447      +/-   ##
==========================================
+ Coverage   97.24%   97.25%   +0.01%     
==========================================
  Files          39       39              
  Lines        8231     8238       +7     
  Branches     1442     1442              
==========================================
+ Hits         8004     8012       +8     
  Misses         98       98              
+ Partials      129      128       -1
Impacted Files Coverage Δ
aiohttp/connector.py 97.5% <100%> (+0.22%) ⬆️

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 7f31c58...0366bc7. Read the comment docs.

@pfreixes
Copy link
Contributor

Yeps, Maybe Im reading in the wrong way but looks like you are trying to implement a kind of failover or circuit breaker pattern inside the logic of the _create_direct_connection function.

IMHO this is a bad idea and it makes the API not predictable. What happen if we wanna log in our app each time that there is an error connecting to a specific IP address ? What to do with the errors at this level has to be done by upper layers, these layers that are in the domain of the developer.

@hellysmile
Copy link
Member Author

asyncio itself has this functionality. Aiohttp had it prior 2.3.0, where it was removed by mistake. I am trying to fix my own mistakes. I was talking with @asvetlov offline regarding logging of failed connections and decision was do not implement it, cuz asyncio do not log it

@hellysmile
Copy link
Member Author

Another solution instead of logging can be raise each new exception from previous one

@pfreixes
Copy link
Contributor

@hellysmile can you give me a link which shows how this is implemented by "asyncio" and another one that shows how it was implemented before by aiohttp?

@hellysmile
Copy link
Member Author

loop.create_connection https://github.com/python/cpython/blob/master/Lib/asyncio/base_events.py#L752

aiohttp 2.2
https://github.com/aio-libs/aiohttp/blob/2.2/aiohttp/connector.py#L733

@pfreixes
Copy link
Contributor

You are right, thanks for the info!

@hellysmile
Copy link
Member Author

@pfreixes can I ask You for a review?

asvetlov
asvetlov requested changes Oct 31, 2017
View reviewed changes
aiohttp/connector.py Outdated
return transp, proto
else:
assert last_exc is not None
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just drop the line.
Check doesn't point on potential problem, next line will fail enyway (raise None is forbidden in Python 3).

asvetlov
asvetlov approved these changes Nov 1, 2017
View reviewed changes
Copy link
Member

@asvetlov asvetlov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@asvetlov asvetlov merged commit 460e719 into aio-libs:2.3 Nov 1, 2017
This was referenced Nov 1, 2017
Closed
Closed
@hellysmile hellysmile deleted the fix_hosts_dns_2.3 branch November 1, 2017 09:32
asvetlov pushed a commit that referenced this pull request Nov 1, 2017
@hellysmile @asvetlov
Merge master to 2.3 (#2456)
caab965 
* Fix wrap oserrors. (#2442)

* Fix wrap oserrors.

* Skip unix socket tests on windows

* Fix wrap ssl errors for proxy connector. (#2446)

* Fix 2451: Rename from_env to trust_env in client reference.

* Fix connection for multiple dns hosts (#2447)

* Proof of concept fix for multiple dns hosts

* Revert getpeercert

* Added test for multiple dns hosts and errors.

* Added change notes.

* Remove redunant assert.

* Minor docs formatting

* Move TOC to separate RST file

* Build tarball on appveyor

* Bump to 2.3.2b1

* @asyncio.coroutine -> async

* Update index.rst

* Update toc.rst
@lock
Copy link

lock bot commented Oct 28, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a [new issue] for related bugs.
If you feel like there's important points made in this discussion, please include those exceprts into that [new issue].
[new issue]: https://github.com/aio-libs/aiohttp/issues/new

@lock lock bot added the outdated label Oct 28, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 28, 2019
@psf-chronographer psf-chronographer bot added the bot:chronographer:provided There is a change note present in this PR label Oct 28, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@asvetlov asvetlov asvetlov approved these changes

Assignees
No one assigned
Labels
bot:chronographer:provided There is a change note present in this PR outdated
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@hellysmile @codecov-io @pfreixes @asvetlov