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.

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.

1 participant