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

Add timeout parameter propagation from lookup func #176

Merged
merged 1 commit into from
Jan 3, 2022
Merged

Add timeout parameter propagation from lookup func #176

merged 1 commit into from
Jan 3, 2022

Conversation

Dominique57
Copy link
Contributor

Closes #173

As mentioned in the issue, the timeout parameter has been added in the lookup functions and propagated to the appropriate functions.

I have done some small functional / visual testing, which are not included. I only tested MinecraftServer since I don't have a BedrockServer and I tested ping, status and query and also the async variants.

During my small functional testing, I observed that if the URL of the server is a domain name, socket.create_connection can take longer than the specified timeout. This is better described here :
https://stackoverflow.com/questions/12232225/python-3-2-3-socket-takes-longer-to-timeout-than-it-should

Copy link
Contributor

@ItsDrike ItsDrike left a comment

Choose a reason for hiding this comment

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

During my small functional testing, I observed that if the URL of the server is a domain name, socket.create_connection can take longer than the specified timeout. This is better described here :
https://stackoverflow.com/questions/12232225/python-3-2-3-socket-takes-longer-to-timeout-than-it-should

I'd say that this is a good thing. It's intentionally implemented like this in standard library socket module and it makes a lot of sense to wait out the timeout for each IP address obtained from resolving until a connection is established, rather than having one global timeout during which the connection has to be made.

Looks good to go to me.

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.

Change default Timeout
3 participants