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

fix(startup): Differentiate between None vs. 0 port #3037

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

DarwinsBuddy
Copy link

I'm currently developing a sanic application which should support service discovery.
In order to have a degree of freedom in choosing how to run sanic in this context, I'm in need of random ports to be chosen by the operating system.
Intentionally, at least according to the docs, this should be possible by simply setting port either to None or 0.
Though, I stumbled upon the mixin/startup code which is in case of a tls start up fixing this to 8443 or 8000 in both occasions, since coalescing port like this
port = port or 8443 if (version == 3 or auto_tls) else 8000

will be in both cases (0 and None) fallback to this term 8443 if (version == 3 or auto_tls) else 8000

Since I do not want to break anything, I suggest changing this to the following behaviour:

In case of port=None persist the logic as is
In case of port=0 let the OS handle choosing a port for us (which would be a random port)

Feedback is very much welcome and I'm open for alternative approaches.
If we could enable this one way or the other I'd very much appreciate it.

(Thank you for this awesome framework ❤️ )

@DarwinsBuddy
Copy link
Author

Unfortunately, I cannot comply to the CONTRIBUTING guidelines, as the parent commit already conflicts with it.
I don't have enough insight on how to fix all of those issues. (some of them I saw are intentional)

If I can do more to kick the can please let me know.

@@ -892,7 +892,8 @@ def get_address(
Tuple[str, int]: Tuple containing the host and port
""" # noqa: E501
host = host or "127.0.0.1"
port = port or (8443 if (version == 3 or auto_tls) else 8000)
if port is None:
Copy link
Member

Choose a reason for hiding this comment

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

I am generally fine with this approach, but I think maybe we should be explicit.

if port is None:
    # use a default
elif port is 0:
    port = _get_random_port()
...

In this case we would bind to get something from the os, but immediately close. I suppose in theory this might fail for some interval, would need to explore that. But I think we want to ensure that get_address does indeed return tuple[str, int]. If not I suspect the changes might need to be deeper here.

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.

2 participants