Fix server not running with explicit hostname #827
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cdeler
reviewed
Oct 21, 2020
uvicorn/config.py
Outdated
| @@ -122,7 +122,7 @@ class _IPKind(Enum): | |||
|
|
|||
|
|
|||
| def _get_server_start_message( | |||
| host_ip_version: _IPKind = _IPKind.IPv4, | |||
| host_ip_version: Optional[_IPKind] = None, | |||
There was a problem hiding this comment.
@euri10
Looks like I was wrong proposing _IPKind enum in #803 . Looks like this function has 2 modes: either "to get a message for ipv6" and "to get a message for ipv4/hostname"
What do you think if we delete the _IPKind enum and change this function signature to
Suggested change
| host_ip_version: Optional[_IPKind] = None, | |
| *, is_ipv6_message: bool = False |
(and also change the function body & usage)
There was a problem hiding this comment.
yup good idea, I didnt see it coming either 🍔
| else: | ||
| message, color_message = _get_server_start_message(_IPKind.IPv4) | ||
| try: | ||
| addr = ip_address(config.host) |
There was a problem hiding this comment.
Looks like you cannot omit this try-except block (or to replace it by regex)
Another approach to check if it's an ip or hostname is to:
socket.getaddrinfo(config.host, 0, socket.AF_UNSPEC, socket.SOCK_STREAM, 0, socket.AI_NUMERICHOST)But it's also should be wrapped by try-except statement
This was referenced Oct 24, 2020
|
For traceability: marking this as a cleanup of #803. |
florimondmanca
added a commit
that referenced
this pull request
Oct 24, 2020
This reverts commit d78394e.
Merged
florimondmanca
added a commit
that referenced
this pull request
Nov 8, 2020
* Revert "Fix server not running with explicit hostname (#827)" This reverts commit d78394e. * Revert "Fix reload with ipv6 host (#803)" This reverts commit 5acaee5. * Rework IPv6 support * Fix IPv6 localhost equivalent Co-authored-by: euri10 <[email protected]> * Reduce diff size * More diff size reduction * Fix: self.host -> config.host Co-authored-by: euri10 <[email protected]>
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #825
I'm not super happy with the try catch on the ip adresss but couldn't think of something else, so if someone has a better idea please share !
Not happy because I'm not sure it accounts for all potential use cases,