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

Support multiple address resolution in DNS requests #49026

Merged
merged 1 commit into from
Jun 1, 2021

Conversation

sarchar
Copy link
Contributor

@sarchar sarchar commented May 24, 2021

Add two new functions to the IP class that returns all addresses/aliases associated with a given address.

This is a cherry-pick merge from 010a343 which was merged in 2.1, and has been updated to build with the latest code.

This merge adds two new methods IP.resolve_hostname_addresses and IP.get_resolve_item_addresses that returns a List of all addresses returned from the DNS request.

@sarchar sarchar requested review from a team as code owners May 24, 2021 13:16
@akien-mga akien-mga added this to the 4.0 milestone May 24, 2021
@akien-mga akien-mga requested a review from a team May 24, 2021 13:18
@fire
Copy link
Member

fire commented May 24, 2021

There is a guide for pr contributions and if you need help with fixing the CICD build failures please message here.

https://docs.godotengine.org/en/stable/community/contributing/pr_workflow.html

core/io/ip.cpp Show resolved Hide resolved
core/io/ip.cpp Outdated Show resolved Hide resolved
drivers/unix/ip_unix.cpp Outdated Show resolved Hide resolved
doc/classes/IP.xml Outdated Show resolved Hide resolved
doc/classes/IP.xml Outdated Show resolved Hide resolved
drivers/unix/ip_unix.h Outdated Show resolved Hide resolved
doc/classes/IP.xml Outdated Show resolved Hide resolved
drivers/unix/ip_unix.cpp Outdated Show resolved Hide resolved
Add two new functions to the IP class that returns all addresses/aliases associated with a given address.

This is a cherry-pick merge from 010a343 which was merged in 2.1, and has been updated to build with the latest code.

This merge adds two new methods IP.resolve_hostname_addresses and IP.get_resolve_item_addresses that returns a List of all addresses returned from the DNS request.
Copy link
Collaborator

@Faless Faless left a comment

Choose a reason for hiding this comment

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

Looks great! 👍

@akien-mga akien-mga merged commit 9990f28 into godotengine:master Jun 1, 2021
@akien-mga
Copy link
Member

Thanks! And congrats for your first merged Godot contribution 🎉

@akien-mga akien-mga added the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Jun 1, 2021
@YuriSizov
Copy link
Contributor

YuriSizov commented Jun 2, 2021

EDIT: Opened an issue #49261, original comment below:


This PR breaks file downloading (e.g. export templates downloading in the editor). The download stops at an unassuming status STATUS_DISCONNECTED:

image

If use_threads is disabled, the status changes to STATUS_RESOLVING, but it still stops at that and never downloads anything.

image

Code that doesn't work:

	download_templates->set_download_file(EditorPaths::get_singleton()->get_cache_dir().plus_file("tmp_templates.tpz"));
	download_templates->set_use_threads(true);

	Error err = download_templates->request(p_url);

But requests like this do work:

	request_mirror->request("https://godotengine.org/mirrorlist/" + p_version + ".json");

Any ideas?

@akien-mga
Copy link
Member

I had a quick go at cherry-picking this + #49269 for 3.x but there's a few conflicts to handle, and even after fixing them I'm not sure that the feature actually worked properly (the Templates tab in the project manager would freeze). So I'd suggest making a dedicated PR backporting both commits if you're interested in having this feature in 3.4+ too.

@sarchar
Copy link
Contributor Author

sarchar commented Jun 7, 2021

I had a quick go at cherry-picking this + #49269 for 3.x but there's a few conflicts to handle, and even after fixing them I'm not sure that the feature actually worked properly (the Templates tab in the project manager would freeze). So I'd suggest making a dedicated PR backporting both commits if you're interested in having this feature in 3.4+ too.

#49020 has that commit; admittedly it isn't up to date with the MutexLock fix (I can do that quickly). Is that one still good enough?

@akien-mga
Copy link
Member

#49020 has that commit; admittedly it isn't up to date with the MutexLock fix (I can do that quickly). Is that one still good enough?

Ah yes, please update it to address feedback from this PR if relevant, and include the regression fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants