Skip to content

feat: Change default listen address to 0.0.0.0 #2307

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.

Sign up for GitHub

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

Merged
simon-lemay-unity merged 15 commits into from
Nov 21, 2022
Merged

feat: Change default listen address to 0.0.0.0 #2307

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
f88ce73
feat: Change default listen address to 0.0.0.0
simon-lemay-unity Nov 14, 2022
79445d8
Add PR number to CHANGELOG entry
simon-lemay-unity Nov 14, 2022
9ae413a
Fix IPv6 handling
simon-lemay-unity Nov 15, 2022
dfefdb7
Fix restart test and improve logging
simon-lemay-unity Nov 15, 2022
2300dcb
Merge branch 'develop' into feat/default-listen-address
simon-lemay-unity Nov 15, 2022
75d2b87
Merge branch 'develop' into feat/default-listen-address
simon-lemay-unity Nov 15, 2022
a3445f0
Merge branch 'develop' into feat/default-listen-address
simon-lemay-unity Nov 16, 2022
69b71b1
Update com.unity.netcode.gameobjects/Tests/Editor/Transports/UnityTra…
simon-lemay-unity Nov 16, 2022
c3b8860
Update com.unity.netcode.gameobjects/Runtime/Transports/UTP/UnityTran…
simon-lemay-unity Nov 16, 2022
15c168a
Address some review feedback
simon-lemay-unity Nov 16, 2022
dff55d4
Merge branch 'develop' into feat/default-listen-address
simon-lemay-unity Nov 16, 2022
7ee44d7
Merge branch 'develop' into feat/default-listen-address
simon-lemay-unity Nov 17, 2022
4d21709
Try to make standards script happy
simon-lemay-unity Nov 17, 2022
07471bb
Merge branch 'develop' into feat/default-listen-address
simon-lemay-unity Nov 21, 2022
3dcac6d
Move the CHANGELOG entry to the next release
simon-lemay-unity Nov 21, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions com.unity.netcode.gameobjects/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ Additional documentation and release notes are available at [Multiplayer Documen

### Changed

- The default listen address of `UnityTransport` is now 0.0.0.0. (#2307)
- Optimized bandwidth usage by encoding most integer fields using variable-length encoding. (#2276)

### Fixed
Expand Down
26 changes: 20 additions & 6 deletions com.unity.netcode.gameobjects/Runtime/Transports/UTP/UnityTransport.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,7 @@ private enum State
// frame at 60 FPS. This will be a large over-estimation in any realistic scenario.
private const int k_MaxReliableThroughput = (NetworkParameterConstants.MTU * 32 * 60) / 1000; // bytes per millisecond

private static ConnectionAddressData s_DefaultConnectionAddressData = new ConnectionAddressData { Address = "127.0.0.1", Port = 7777, ServerListenAddress = string.Empty };
private static ConnectionAddressData s_DefaultConnectionAddressData = new ConnectionAddressData { Address = "127.0.0.1", Port = 7777, ServerListenAddress = "0.0.0.0" };

#pragma warning disable IDE1006 // Naming Styles

Expand Down Expand Up @@ -303,9 +303,9 @@ public struct ConnectionAddressData
public ushort Port;

/// <summary>
/// IP address the server will listen on. If not provided, will use 'Address'.
/// IP address the server will listen on. If not provided, will use 0.0.0.0.
/// </summary>
[Tooltip("IP address the server will listen on. If not provided, will use 'Address'.")]
[Tooltip("IP address the server will listen on. If not provided, will use 0.0.0.0.")]
[SerializeField]
public string ServerListenAddress;

Expand All @@ -330,7 +330,21 @@ private static NetworkEndpoint ParseNetworkEndpoint(string ip, ushort port)
/// <summary>
/// Endpoint (IP address and port) server will listen/bind on.
/// </summary>
public NetworkEndpoint ListenEndPoint => ParseNetworkEndpoint((ServerListenAddress?.Length == 0) ? Address : ServerListenAddress, Port);
public NetworkEndpoint ListenEndPoint
{
get
{
if (ServerListenAddress?.Length == 0)
Copy link
Copy Markdown
Contributor

@JesseOlmer JesseOlmer Nov 16, 2022

Choose a reason for hiding this comment

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

Won't this log Debug.LogError($"Invalid network endpoint: {ip}:{port}."); in the case where the server listen address is empty?

I don't see an actual test case for this conditional - do we have one for an empty address versus a malformed one?

Copy link
Copy Markdown
Contributor

@JesseOlmer JesseOlmer Nov 16, 2022

Choose a reason for hiding this comment

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

Rather, line 339 (accessing ServerEndPoint) would log this in this case.

Copy link
Copy Markdown
Contributor Author

@simon-lemay-unity simon-lemay-unity Nov 16, 2022

Choose a reason for hiding this comment

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

Yes, if both the 'Address' and 'Listen Address' fields are left empty, it will log an error. Although that scenario was never supported before. The previous behavior if leaving 'Listen Address' empty was to use 'Address'. I can add a special case where if both are left empty, we revert to 0.0.0.0 as the listen address.

There are no specific tests for leaving the listen address empty, but that is what a lot of tests actually end up doing, since it's the default when calling SetConnectionData without explicitly setting a listen address.

Copy link
Copy Markdown
Contributor Author

@simon-lemay-unity simon-lemay-unity Nov 16, 2022

Choose a reason for hiding this comment

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

Pushed a change that avoids an error if both 'Address' and 'Listen Address' are empty (we'll then default to 0.0.0.0). Also added a test case for this particular scenario.

{
var ep = ServerEndPoint.Family == NetworkFamily.Ipv4 ? NetworkEndpoint.AnyIpv4 : NetworkEndpoint.AnyIpv6;
return ep.WithPort(Port);
}
else
{
return ParseNetworkEndpoint(ServerListenAddress, Port);
}
}
}
}

/// <summary>
Expand Down Expand Up @@ -529,14 +543,14 @@ private bool ServerBindAndListen(NetworkEndpoint endPoint)
int result = m_Driver.Bind(endPoint);
if (result != 0)
{
Debug.LogError("Server failed to bind");
Debug.LogError("Server failed to bind. This is usually caused by another processing being already bound to the same port.");
Comment thread
simon-lemay-unity marked this conversation as resolved.
Outdated
return false;
}

result = m_Driver.Listen();
if (result != 0)
{
Debug.LogError("Server failed to listen");
Debug.LogError("Server failed to listen.");
return false;
}

Expand Down
6 changes: 3 additions & 3 deletions com.unity.netcode.gameobjects/Tests/Editor/Transports/UnityTransportTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,13 +115,13 @@ public void UnityTransport_RestartSucceedsAfterFailure()
UnityTransport transport = new GameObject().AddComponent<UnityTransport>();
transport.Initialize();

transport.SetConnectionData("127.0.0.", 4242);
transport.SetConnectionData("127.0.0.", 4242, "127.0.0.");
Assert.False(transport.StartServer());

LogAssert.Expect(LogType.Error, "Invalid network endpoint: 127.0.0.:4242.");
LogAssert.Expect(LogType.Error, "Server failed to bind");
LogAssert.Expect(LogType.Error, "Server failed to bind. This is usually caused by another processing being already bound to the same port.");
Comment thread
simon-lemay-unity marked this conversation as resolved.
Outdated

transport.SetConnectionData("127.0.0.1", 4242);
transport.SetConnectionData("127.0.0.1", 4242, "127.0.0.1");
Assert.True(transport.StartServer());

transport.Shutdown();
Expand Down