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

Revert "Set socket family of connector to AF_INET" #9870

Merged
merged 1 commit into from
Aug 28, 2024

Conversation

oliver-ni
Copy link
Contributor

@oliver-ni oliver-ni commented Jun 25, 2024

This change was made since Discord doesn't support IPv6, and there were concerns about clients with DNS64 enabled without NAT64.

However, this breaks hosts who don't have v4 connectivity and are actually running NAT64 (me!)

Having DNS64 without NAT64 is really an issue on the client's end. It would break far more than just discord.py, so I don't think we should be concerned about those cases. I'm proposing this change be reverted.

@oliver-ni oliver-ni force-pushed the master branch 2 times, most recently from 842f216 to 035e26b Compare June 25, 2024 03:49
This change was made since Discord doesn't support IPv6, and there were
concerns about clients with DNS64 enabled without NAT64.

However, this breaks hosts who don't have v4 connectivity and are
_actually_ running NAT64.

Having DNS64 without NAT64 is really an issue on the client's end. It
would break far more than just discord.py, so I don't think we should be
concerned about those cases.
Copy link
Contributor

@SebbyLaw SebbyLaw left a comment

Choose a reason for hiding this comment

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

LGTM

@Rapptz
Copy link
Owner

Rapptz commented Jun 25, 2024

I'm not a fan of this solution. This should probably be a Client setting instead with the default being False.

@mikeshardmind
Copy link
Contributor

mikeshardmind commented Jun 25, 2024

Users can (Actually, this appears to have been removed, see now linked PR) pass in a connector already, having this be set to AF_INET by default causes the error in the more complex networking case to at least make sense, the error users would get without this in the failing case was not useful, see error from the original issue, maybe this should just be solved with additional documentation that if you have NAT64 without ipv4 you need to also have DNS64 and pass in your own connector.

mikeshardmind added a commit to mikeshardmind/discord.py that referenced this pull request Aug 5, 2024
This provides paths for users to handle two entirely seperate issues

- Alternative fix for Rapptz#9870
- Allows handling of windows sslcontext issues without a global
truststore.inject_into_ssl() use
mikeshardmind added a commit to mikeshardmind/discord.py that referenced this pull request Aug 5, 2024
This provides paths for users to handle two entirely seperate issues

- Alternative fix for Rapptz#9870
- Allows handling of windows sslcontext issues without a global
truststore.inject_into_ssl() use
mikeshardmind added a commit to mikeshardmind/discord.py that referenced this pull request Aug 5, 2024
This provides paths for users to handle two entirely seperate issues

- Alternative fix for Rapptz#9870
- Allows handling of windows sslcontext issues without a global
truststore.inject_into_ssl() use
@mikeshardmind mikeshardmind mentioned this pull request Aug 5, 2024
6 tasks
mikeshardmind added a commit to mikeshardmind/discord.py that referenced this pull request Aug 5, 2024
This provides paths for users to handle two entirely seperate issues

- Alternative fix for Rapptz#9870
- Allows handling of windows sslcontext issues without a global
truststore.inject_into_ssl() use
Rapptz pushed a commit that referenced this pull request Aug 28, 2024
This provides paths for users to handle two entirely seperate issues

- Alternative fix for #9870
- Allows handling of windows sslcontext issues without a global
truststore.inject_into_ssl() use
@Rapptz
Copy link
Owner

Rapptz commented Aug 28, 2024

I've given this some more thought and I'm just going to go ahead and allow it.

@Rapptz Rapptz merged commit f9dbe60 into Rapptz:master Aug 28, 2024
parelite added a commit to parelite/discord.py that referenced this pull request Aug 29, 2024
* Fix documentation for abc.GuildChannel.move to be more clear

* Fix Polls limiting duration at 7 days

* Fix _get_command_error improperly handling some error messages

* Fix introduced potential TypeError with _get_command_error

* Re-add client connector param

This provides paths for users to handle two entirely seperate issues

- Alternative fix for Rapptz#9870
- Allows handling of windows sslcontext issues without a global
truststore.inject_into_ssl() use

* Fix Poll.duration rounding error

* Add support for getting the attachment's title

* Fix default_avatar for team user and webhook

* [docs] Remove pack_id attribute from Sticker

[docs] Remove unnecessary pack_id

* Fix url for GIF StickerItem

* [docs] correct hyperlink to discord docs

* [docs] Fix typehint for Embed.set_(image,thumbnail)

* Remove stale documentation in Embed.set_thumbnail

* Add Guild.fetch_role

* Add approximate_user_install_count to AppInfo

* Revert "Set socket family of connector to AF_INET"

This change was made since Discord doesn't support IPv6, and there were
concerns about clients with DNS64 enabled without NAT64.

However, this breaks hosts who don't have v4 connectivity and are
_actually_ running NAT64.

Having DNS64 without NAT64 is really an issue on the client's end. It
would break far more than just discord.py, so I don't think we should be
concerned about those cases.

* Add support for get sticker pack

* Fix stacklevel for Message.interaction deprecation warning

* Use fallback audioop package for Python v3.13 or higher

Fix Rapptz#9477

* Add DummyCookieJar to client owned ClientSession

* Add missing guild incident fields

Co-authored-by: owocado <[email protected]>
Co-authored-by: Danny <[email protected]>

* Add support Member.fetch_voice

* Add support for application emojis

Co-authored-by: DA344 <[email protected]>
Co-authored-by: Danny <[email protected]>

* Add a warning if interaction endpoint URL is set on login

* Remove aiodns from being used on Windows

---------

Co-authored-by: Rapptz <[email protected]>
Co-authored-by: DA344 <[email protected]>
Co-authored-by: Pipythonmc <[email protected]>
Co-authored-by: Michael H <[email protected]>
Co-authored-by: ambdroid <[email protected]>
Co-authored-by: Soheab <[email protected]>
Co-authored-by: owocado <[email protected]>
Co-authored-by: Andrin <[email protected]>
Co-authored-by: Gaurav <[email protected]>
Co-authored-by: fretgfr <[email protected]>
Co-authored-by: lmaotrigine <[email protected]>
Co-authored-by: Alex Nørgaard <[email protected]>
Co-authored-by: Oliver Ni <[email protected]>
Co-authored-by: Jakub Kuczys <[email protected]>
Co-authored-by: Deep Jain <[email protected]>
Co-authored-by: Danny <[email protected]>
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.

4 participants