-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
🔗 Fix connecting to link-local IPv6 addresses #4556
Conversation
@cooperlees I would recommend you to use IPv6 ULA addresses. I have painful experience with LL addresses. Especially in Avahi project. So, anyway, this fix should work. Please check. |
aiodns uses pycares. Unfortunately, pycares is an ancient library with obsolete (and definitely buggy) code. I would drop using pycares anywhere. It's much better to move code with resolving using threads to aiodns. Would you like it ? |
@socketpair mind filling out the PR template? |
hosts.append({ | ||
'hostname': host, | ||
'host': host, | ||
'port': port, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe normalize the port here instead of the if-block?
'port': port, | |
'port': int(port), |
@@ -0,0 +1 @@ | |||
Fix connecting to link-local IPv6 addresses. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any chance you could add a test for this?
@socketpair bump |
Hey, is there still interest in getting this merged? I work on a downstream project (Ray) and we've had a lot of users come across this problem. If there's interest in getting this merged, I think we'd be willing to fix up this PR. (btw this is almost objectively no longer a rare issue, we've seen it on reproduced on popular ubuntu and redhat distros) |
I’m happy to lend some cycles to polish this up to if it will be accepted. |
@webknjaz bump |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
wow |
I've finally found time for the project :) |
Codecov Report
@@ Coverage Diff @@
## master #4556 +/- ##
==========================================
- Coverage 97.53% 97.50% -0.04%
==========================================
Files 43 43
Lines 8979 8983 +4
Branches 1417 1418 +1
==========================================
+ Hits 8758 8759 +1
- Misses 101 103 +2
- Partials 120 121 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
💚 Backport successfulThe PR was backported to the following branches:
|
What do these changes do?
Are there changes in behavior for the user?
Related issue number
Fixes #4554
Checklist
CONTRIBUTORS.txt
CHANGES
folder<issue_id>.<type>
for example (588.bugfix)issue_id
change it to the pr id after creating the pr.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.