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

[Net] Fix IP address resolution incorrectly locking the main thread. #51199

Merged
merged 1 commit into from
Aug 3, 2021

Conversation

Faless
Copy link
Collaborator

@Faless Faless commented Aug 2, 2021

This seems to be a pretty old bug, older then originally reported (at least under certain circumstances).

The IP singleton uses a resolve queue so developers can queue hostnames for resolution in a separate while keeping the main thread unlocked (address-resolution OS functions are blocking, and could block for a long time in case of network disruption).

In most places though, the address resolution function was called with the mutex locked, causing other functions (querying status, queueing another hostname, ecc) to block until that resolution ended.

This commit ensures that all calls to OS address resolution are done with the mutex unlocked.

Fixes #51181 . Note: wile in the issue this is described as a regression, this is a very old issue (according to my tests), according to my tests on Linux, and can be found even to 2.1. I suspect testing it is just tricky due to OS/router DNS caches.

3.x version here

This seems to be a pretty old bug, older then originally reported (at
least under certain circumstances).

The IP singleton uses a resolve queue so developers can queue hostnames
for resolution in a separate while keeping the main thread unlocked
(address-resolution OS functions are blocking, and could block for a long
time in case of network disruption).

In most places though, the address resolution function was called with
the mutex locked, causing other functions (querying status, queueing
another hostname, ecc) to block until that resolution ended.

This commit ensures that all calls to OS address resolution are done
with the mutex unlocked.
@tavurth
Copy link
Contributor

tavurth commented Aug 2, 2021

Wow so fast 🎉

@akien-mga akien-mga merged commit 3c21fc5 into godotengine:master Aug 3, 2021
@akien-mga
Copy link
Member

Thanks!

@akien-mga akien-mga added cherrypick:3.3 cherrypick:3.x Considered for cherry-picking into a future 3.x release labels Aug 3, 2021
@akien-mga
Copy link
Member

Cherry-picked for 3.4.

@akien-mga akien-mga removed the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Aug 3, 2021
@akien-mga
Copy link
Member

Fixes #51181 . Note: wile in the issue this is described as a regression, this is a very old issue (according to my tests), according to my tests on Linux, and can be found even to 2.1. I suspect testing it is just tricky due to OS/router DNS caches.

Isn't this a regression from #49026? If so that would explain why it's present in 2.1, as this change was made in 2.1 and not master until #49026, which was then backported to 3.x.

If so, the 3.3 branch shouldn't be affected (and indeed the fix doesn't seem to cherry-pick cleanly at least).

@Faless
Copy link
Collaborator Author

Faless commented Aug 3, 2021

3.2 seems to also have the underlying bug, though maybe harder to get to (would still break if you had to HTTPRequest at least):

https://github.com/godotengine/godot/blob/3.2/core/io/ip.cpp#L99-L106

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.

HTTPRequest freezes when hostname resolution stalls due to broken connectivity.
4 participants