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

Support Ephemeral ports #5126

Draft
wants to merge 7 commits into
base: develop
Choose a base branch
from

Conversation

fishface60
Copy link
Contributor

@fishface60 fishface60 commented Dec 27, 2024

Identify the Bug or Feature request

Closes #5125

Depends on #5124 or #5131

Description of the Change

This relaxes the validation of the server port field to allow 0, changes code that used the configured port for sending to the registry or advertising with service discovery to use the value from the bound socket, and ensures they are configured after the socket has been bound so the port can be correctly determined.

Possible Drawbacks

If a user specifies it as 0 without using UPnP then connections will probably fail and those error messages are less informative than having picked an invalid port.

Release Notes

  • The server port may be specified as 0 to use a random port that may be mapped with UPnP and configured with the rptools.net server registry or advertised on LAN.

This change is Reviewable

@github-actions github-actions bot added the feature Adding functionality that adds value label Dec 27, 2024
@fishface60 fishface60 force-pushed the ephemeral-ports branch 3 times, most recently from 2db5327 to 0b3b2b9 Compare December 28, 2024 18:37
Sealed interfaces are useful in switch exprs to enumerate possible values
so that it becomes a compile error to introduce a new server type
without implementing an appropriate arm for the switch block.
ServiceAnnouncer requires a non-null String, but an empty server ID is
not well-formed so a ServiceAnnouncer shouldn't be created.

This was previously handled by MapToolServer being passed null config
and the absence of a port to give was sufficient to prevent creating a
ServiceAnnouncer with an empty ID,
but we can be more defensive about this and check for a null pointer.
This adds extra complexity from the ServiceAnnouncer not always existing
but enables later flexibility in supporting binding to an emphemeral
port instead of having to preconfigure it.
It is neater to stop announcement, deregister and close UPnP
before stopping the server so that discovering clients
can't try to connect to an already stopped server
though this is mostly just being picky because the human interaction
time scales dominate and shut down and deregister are nearly the same
time.
If a port number of 0 is chosen then bind() will open a random port in
an ephemeral range instead of the provided port number
and this port can then be retrieved using getLocalPort().

This enables starting servers on arbitrary ports instead of
preconfigured ones.

Since only direct connections need to know the port number
connections using the registry or LAN can use an ephemeral port number
and the connection information tab can display the port direct
connections should use.

This requires a router with functioning UPnP instead of port forwarding
though so is not univerally useful.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Adding functionality that adds value
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature]: Support starting MapTool on an ephemeral port instead of configuring in advance
1 participant