From 8159db7641efbdde72ee728f3e4ed1c50fa03174 Mon Sep 17 00:00:00 2001 From: Milon Date: Tue, 21 Jan 2025 14:23:50 +0100 Subject: [PATCH 1/4] fix --- Robust.Client/BaseClient.cs | 4 ---- Robust.Shared/Network/NetManager.cs | 15 +++++++++++++++ 2 files changed, 15 insertions(+), 4 deletions(-) diff --git a/Robust.Client/BaseClient.cs b/Robust.Client/BaseClient.cs index 09a786020ce..4445e63db9d 100644 --- a/Robust.Client/BaseClient.cs +++ b/Robust.Client/BaseClient.cs @@ -115,10 +115,6 @@ public void ConnectToServer(DnsEndPoint endPoint) /// public void DisconnectFromServer(string reason) { - DebugTools.Assert(RunLevel > ClientRunLevel.Initialize); - DebugTools.Assert(_net.IsConnected); - // run level changed in OnNetDisconnect() - // are both of these *really* needed? _net.ClientDisconnect(reason); } diff --git a/Robust.Shared/Network/NetManager.cs b/Robust.Shared/Network/NetManager.cs index 4c7f9002ca2..a1a77380d70 100644 --- a/Robust.Shared/Network/NetManager.cs +++ b/Robust.Shared/Network/NetManager.cs @@ -587,6 +587,21 @@ public void ProcessPackets() public void ClientDisconnect(string reason) { DebugTools.Assert(IsClient, "Should never be called on the server."); + + // First handle any in-progress connection attempt + if (ClientConnectState != ClientConnectionState.NotConnecting) + { + _cancelConnectTokenSource?.Cancel(); + + // Clean up any pending net peers + foreach (var peer in _netPeers) + { + peer.Peer.Shutdown(reason); + _toCleanNetPeers.Add(peer.Peer); + } + } + + // Then handle existing connection if any if (ServerChannel != null) { Disconnect?.Invoke(this, new NetDisconnectedArgs(ServerChannel, reason)); From 09158e09bf2abbaa0b30a0bf69ba35440f3f4263 Mon Sep 17 00:00:00 2001 From: Milon Date: Wed, 22 Jan 2025 15:43:44 +0100 Subject: [PATCH 2/4] actually fix for real this time --- Robust.Shared/Network/NetManager.ClientConnect.cs | 15 +++++++++++---- Robust.Shared/Network/NetManager.cs | 7 ------- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/Robust.Shared/Network/NetManager.ClientConnect.cs b/Robust.Shared/Network/NetManager.ClientConnect.cs index 7e8a4d3c9cc..02a31941413 100644 --- a/Robust.Shared/Network/NetManager.ClientConnect.cs +++ b/Robust.Shared/Network/NetManager.ClientConnect.cs @@ -320,11 +320,18 @@ async Task AwaitNonInitStatusChange(NetConnection connection, Cancellati NetConnectionStatus status; string reason; - do + try { - reason = await AwaitStatusChange(connection, cancellationToken); - status = connection.Status; - } while (status == NetConnectionStatus.InitiatedConnect); + do + { + reason = await AwaitStatusChange(connection, cancellationToken); + status = connection.Status; + } while (status == NetConnectionStatus.InitiatedConnect); + } + catch (OperationCanceledException) + { + reason = "Connection attempt cancelled"; + } return reason; } diff --git a/Robust.Shared/Network/NetManager.cs b/Robust.Shared/Network/NetManager.cs index a1a77380d70..05ff33e9d97 100644 --- a/Robust.Shared/Network/NetManager.cs +++ b/Robust.Shared/Network/NetManager.cs @@ -592,13 +592,6 @@ public void ClientDisconnect(string reason) if (ClientConnectState != ClientConnectionState.NotConnecting) { _cancelConnectTokenSource?.Cancel(); - - // Clean up any pending net peers - foreach (var peer in _netPeers) - { - peer.Peer.Shutdown(reason); - _toCleanNetPeers.Add(peer.Peer); - } } // Then handle existing connection if any From 69e7ac56c4a66211746e5c334e57eb3b0fc945c5 Mon Sep 17 00:00:00 2001 From: Milon Date: Thu, 30 Jan 2025 16:08:04 +0100 Subject: [PATCH 3/4] just use HappyEyeballsHttp :godo: --- .../Network/NetManager.ClientConnect.cs | 212 ++++++------------ 1 file changed, 73 insertions(+), 139 deletions(-) diff --git a/Robust.Shared/Network/NetManager.ClientConnect.cs b/Robust.Shared/Network/NetManager.ClientConnect.cs index 02a31941413..deeb87dcc25 100644 --- a/Robust.Shared/Network/NetManager.ClientConnect.cs +++ b/Robust.Shared/Network/NetManager.ClientConnect.cs @@ -1,6 +1,5 @@ using System; using System.Collections.Generic; -using System.Collections.Immutable; using System.Linq; using System.Net; using System.Net.Http; @@ -11,7 +10,6 @@ using System.Threading; using System.Threading.Tasks; using Lidgren.Network; -using Robust.Shared.Log; using Robust.Shared.Network.Messages.Handshake; using Robust.Shared.Utility; using SpaceWizards.Sodium; @@ -98,7 +96,7 @@ public async void ClientConnect(string host, int port, string userNameRequest) { await CCDoHandshake(winningPeer, winningConnection, userNameRequest, mainCancelToken); } - catch (TaskCanceledException) + catch (Exception e) when (e is OperationCanceledException or TaskCanceledException) { winningPeer.Peer.Shutdown("Cancelled"); _toCleanNetPeers.Add(winningPeer.Peer); @@ -120,7 +118,10 @@ public async void ClientConnect(string host, int port, string userNameRequest) _logger.Debug("Handshake completed, connection established."); } - private async Task CCDoHandshake(NetPeerData peer, NetConnection connection, string userNameRequest, + private async Task CCDoHandshake( + NetPeerData peer, + NetConnection connection, + string userNameRequest, CancellationToken cancel) { var encrypt = _config.GetCVar(CVars.NetEncrypt); @@ -289,171 +290,91 @@ private byte[] MakeAuthHash(byte[] sharedSecret, byte[] pkBytes) private async Task<(NetPeerData winningPeer, NetConnection winningConnection)?> CCHappyEyeballs(int port, IPAddress first, IPAddress? second, CancellationToken mainCancelToken) { - NetPeerData CreatePeerForIp(IPAddress address) + // Try to establish a connection with an IP address and wait for it to either connect or fail + // Returns a disposable wrapper around the peer/connection because ParallelTask + async Task AttemptConnection(IPAddress address, CancellationToken cancel) { var config = _getBaseNetPeerConfig(); - if (address.AddressFamily == AddressFamily.InterNetworkV6) - { - config.LocalAddress = IPAddress.IPv6Any; - } - else - { - config.LocalAddress = IPAddress.Any; - } + config.LocalAddress = address.AddressFamily == AddressFamily.InterNetworkV6 + ? IPAddress.IPv6Any + : IPAddress.Any; var peer = new NetPeer(config); peer.Start(); - var data = new NetPeerData(peer); - _netPeers.Add(data); - return data; - } + var peerData = new NetPeerData(peer); + _netPeers.Add(peerData); - // Create first peer. - var firstPeer = CreatePeerForIp(first); - var firstConnection = firstPeer.Peer.Connect(new IPEndPoint(first, port)); - NetPeerData? secondPeer = null; - NetConnection? secondConnection = null; - string? secondReason = null; - - async Task AwaitNonInitStatusChange(NetConnection connection, CancellationToken cancellationToken) - { - NetConnectionStatus status; - string reason; + var connection = peer.Connect(new IPEndPoint(address, port)); try { - do + // We need AwaitNonInitStatusChange to properly handle connection state transitions + var reason = await AwaitNonInitStatusChange(connection, cancel); + + if (connection.Status != NetConnectionStatus.Connected) { - reason = await AwaitStatusChange(connection, cancellationToken); - status = connection.Status; - } while (status == NetConnectionStatus.InitiatedConnect); + // Connection failed, clean up and yeet an exception + peer.Shutdown(reason); + _toCleanNetPeers.Add(peer); + throw new Exception($"Connection failed: {reason}"); + } + + return new ConnectionAttempt(peerData, connection); } - catch (OperationCanceledException) + catch (Exception) { - reason = "Connection attempt cancelled"; + // Something went wrong! + peer.Shutdown("Connection attempt failed"); + _toCleanNetPeers.Add(peer); + throw; } - - return reason; } - async Task ConnectSecondDelayed(CancellationToken cancellationToken) + // Waits for a connection's status to change from InitiatedConnect to anything else + async Task AwaitNonInitStatusChange(NetConnection connection, CancellationToken cancellationToken) { - DebugTools.AssertNotNull(second); - // Connecting via second peer is delayed by 25ms to give an advantage to IPv6, if it works. - var delay = TimeSpan.FromSeconds(_config.GetCVar(CVars.NetHappyEyeballsDelay)); - await Task.Delay(delay, cancellationToken); - if (cancellationToken.IsCancellationRequested) + string reason; + NetConnectionStatus status; + do { - return; - } + reason = await AwaitStatusChange(connection, cancellationToken); + status = connection.Status; + } while (status == NetConnectionStatus.InitiatedConnect); - secondPeer = CreatePeerForIp(second); - secondConnection = secondPeer.Peer.Connect(new IPEndPoint(second, port)); - - secondReason = await AwaitNonInitStatusChange(secondConnection, cancellationToken); + return reason; } - NetPeerData? winningPeer; - NetConnection? winningConnection; - string? firstReason = null; try { - if (second != null) - { - // We have two addresses to try. - var cancellation = CancellationTokenSource.CreateLinkedTokenSource(mainCancelToken); - var firstPeerChanged = AwaitNonInitStatusChange(firstConnection, cancellation.Token); - var secondPeerChanged = ConnectSecondDelayed(cancellation.Token); + // Create list of IPs to try + var addresses = second != null + ? new[] { first, second } + : new[] { first }; - var firstChange = await Task.WhenAny(firstPeerChanged, secondPeerChanged); + // Use ParallelTask to handle the connection attempts + var delay = TimeSpan.FromSeconds(_config.GetCVar(CVars.NetHappyEyeballsDelay)); + var (result, _) = await HappyEyeballsHttp.ParallelTask( + addresses.Length, + (i, token) => AttemptConnection(addresses[i], token), + delay, + mainCancelToken); - if (firstChange == firstPeerChanged) - { - _logger.Debug("First peer status changed."); - // First peer responded first. - if (firstConnection.Status == NetConnectionStatus.Connected) - { - // First peer won! - _logger.Debug("First peer succeeded."); - cancellation.Cancel(); - if (secondPeer != null) - { - secondPeer.Peer.Shutdown("First connection attempt won."); - _toCleanNetPeers.Add(secondPeer.Peer); - } - - winningPeer = firstPeer; - winningConnection = firstConnection; - } - else - { - // First peer failed, try the second one I guess. - _logger.Debug("First peer failed."); - firstPeer.Peer.Shutdown("You failed."); - _toCleanNetPeers.Add(firstPeer.Peer); - firstReason = await firstPeerChanged; - await secondPeerChanged; - winningPeer = secondPeer; - winningConnection = secondConnection; - } - } - else - { - if (secondConnection!.Status == NetConnectionStatus.Connected) - { - // Second peer won! - _logger.Debug("Second peer succeeded."); - cancellation.Cancel(); - firstPeer.Peer.Shutdown("Second connection attempt won."); - _toCleanNetPeers.Add(firstPeer.Peer); - winningPeer = secondPeer; - winningConnection = secondConnection; - } - else - { - // First peer failed, try the second one I guess. - _logger.Debug("Second peer failed."); - secondPeer!.Peer.Shutdown("You failed."); - _toCleanNetPeers.Add(secondPeer.Peer); - firstReason = await firstPeerChanged; - winningPeer = firstPeer; - winningConnection = firstConnection; - } - } - } - else - { - // Only one address to try. Pretty straight forward. - firstReason = await AwaitNonInitStatusChange(firstConnection, mainCancelToken); - winningPeer = firstPeer; - winningConnection = firstConnection; - } + return (result.Peer, result.Connection); } - catch (TaskCanceledException) + catch (Exception e) when (e is OperationCanceledException or TaskCanceledException) { - firstPeer.Peer.Shutdown("Cancelled"); - _toCleanNetPeers.Add(firstPeer.Peer); - if (secondPeer != null) - { - // ReSharper disable once PossibleNullReferenceException - secondPeer.Peer.Shutdown("Cancelled"); - _toCleanNetPeers.Add(secondPeer.Peer); - } - + // Connection attempt was cancelled, nothing to see here + OnConnectFailed("Connection attempt cancelled."); return null; } - - // winningPeer can still be failed at this point. - // If it is, neither succeeded. RIP. - if (winningConnection!.Status != NetConnectionStatus.Connected) + catch (AggregateException ae) { - winningPeer!.Peer.Shutdown("You failed"); - _toCleanNetPeers.Add(winningPeer.Peer); - OnConnectFailed((secondReason ?? firstReason)!); + // ParallelTask throws AggregateException with all connection failures + // We just take the first one + var message = ae.InnerExceptions.First().Message; + OnConnectFailed(message); return null; } - - return (winningPeer!, winningConnection); } private Task AwaitStatusChange(NetConnection connection, CancellationToken cancellationToken = default) @@ -478,7 +399,8 @@ private Task AwaitStatusChange(NetConnection connection, CancellationTok return tcs.Task; } - private Task AwaitData(NetConnection connection, + private Task AwaitData( + NetConnection connection, CancellationToken cancellationToken = default) { if (_awaitingData.ContainsKey(connection)) @@ -536,5 +458,17 @@ private Task AwaitData(NetConnection connection, } private sealed record JoinRequest(string Hash, string? Hwid); + + [Virtual] + private class ConnectionAttempt(NetPeerData peer, NetConnection connection) : IDisposable + { + public NetPeerData Peer { get; } = peer; + public NetConnection Connection { get; } = connection; + + public void Dispose() + { + Peer.Peer.Shutdown("Disposing unused connection attempt"); + } + } } } From 6552cbd171f538c536945d7a442cef145ca857c9 Mon Sep 17 00:00:00 2001 From: Milon Date: Sat, 22 Feb 2025 22:45:22 +0100 Subject: [PATCH 4/4] review --- Robust.Shared/Network/NetManager.ClientConnect.cs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/Robust.Shared/Network/NetManager.ClientConnect.cs b/Robust.Shared/Network/NetManager.ClientConnect.cs index deeb87dcc25..238bc9d0aa6 100644 --- a/Robust.Shared/Network/NetManager.ClientConnect.cs +++ b/Robust.Shared/Network/NetManager.ClientConnect.cs @@ -96,7 +96,7 @@ public async void ClientConnect(string host, int port, string userNameRequest) { await CCDoHandshake(winningPeer, winningConnection, userNameRequest, mainCancelToken); } - catch (Exception e) when (e is OperationCanceledException or TaskCanceledException) + catch (OperationCanceledException) { winningPeer.Peer.Shutdown("Cancelled"); _toCleanNetPeers.Add(winningPeer.Peer); @@ -319,7 +319,7 @@ async Task AttemptConnection(IPAddress address, CancellationT throw new Exception($"Connection failed: {reason}"); } - return new ConnectionAttempt(peerData, connection); + return new ConnectionAttempt(peerData, connection, this); } catch (Exception) { @@ -361,7 +361,7 @@ async Task AwaitNonInitStatusChange(NetConnection connection, Cancellati return (result.Peer, result.Connection); } - catch (Exception e) when (e is OperationCanceledException or TaskCanceledException) + catch (OperationCanceledException) { // Connection attempt was cancelled, nothing to see here OnConnectFailed("Connection attempt cancelled."); @@ -459,8 +459,7 @@ private Task AwaitData( private sealed record JoinRequest(string Hash, string? Hwid); - [Virtual] - private class ConnectionAttempt(NetPeerData peer, NetConnection connection) : IDisposable + private sealed class ConnectionAttempt(NetPeerData peer, NetConnection connection, NetManager netManager) : IDisposable { public NetPeerData Peer { get; } = peer; public NetConnection Connection { get; } = connection; @@ -468,6 +467,7 @@ private class ConnectionAttempt(NetPeerData peer, NetConnection connection) : ID public void Dispose() { Peer.Peer.Shutdown("Disposing unused connection attempt"); + netManager._toCleanNetPeers.Add(Peer.Peer); } } }