diff --git a/Source/Client/Networking/NetworkingSteam.cs b/Source/Client/Networking/NetworkingSteam.cs index 3f9534a61..1bcf986c0 100644 --- a/Source/Client/Networking/NetworkingSteam.cs +++ b/Source/Client/Networking/NetworkingSteam.cs @@ -2,6 +2,7 @@ using System.Collections.Generic; using System.Diagnostics; using System.Linq; +using System.Threading.Tasks; using Multiplayer.Common; using Multiplayer.Common.Networking.Packet; using Steamworks; @@ -17,6 +18,19 @@ public abstract class SteamBaseConn(CSteamID remoteId, ushort recvChannel, ushor public readonly ushort recvChannel = recvChannel; // currently only for client public readonly ushort sendChannel = sendChannel; // currently only for server + // Time given to Steam to flush a queued goodbye packet before the P2P session is freed (#843). + private static readonly TimeSpan SteamGoodbyeFlushDelay = TimeSpan.FromSeconds(3); + + public override object? RemoteIdentity => remoteId; + + // Steam carries every connection with a peer over one P2P session keyed by their id, so a + // replacement from the same id arrived on this very session and closing would take it down too. + public override void CloseReplacedBy(ConnectionBase replacement, MpDisconnectReason reason) + { + if (replacement is SteamBaseConn conn && conn.remoteId == remoteId) return; + base.CloseReplacedBy(replacement, reason); + } + protected override void SendRaw(byte[] raw, bool reliable = true) { byte[] full = new byte[1 + raw.Length]; @@ -42,12 +56,48 @@ public void SendRawSteam(byte[] raw, bool reliable) public abstract void OnError(EP2PSessionError error); + // A goodbye is only ever non-null server-side, and CloseP2PSessionWithUser discards queued unsent + // packets. Closing immediately would drop the just-queued goodbye (e.g. a wrong-password or kick + // reason) before Steam flushes it, so defer the close to let the reliable packet reach the client. protected override void OnClose(ServerDisconnectPacket? goodbye) { - if (goodbye.HasValue) Send(goodbye.Value); - // TODO this should probably include SteamNetworking.CloseP2PSessionWithUser to free up any leftover - // resources in the Steam API. The API docs are not clear whether the connection is closed instantly, or - // are the queued packets sent. + if (!goodbye.HasValue) + { + CloseSteamSession(); + return; + } + + Send(goodbye.Value); + + var server = serverPlayer.Server; + var id = remoteId; + + Task.Delay(SteamGoodbyeFlushDelay).ContinueWith(_ => server.Enqueue(() => + { + // A fast reconnect (SteamP2PNetManager.Tick) reuses this same remoteId on a fresh + // connection during the delay window. Closing then would tear that new session down, so + // skip the close if another connection already replaced this one (#843). + if (server.playerManager.Players.Any(p => p.conn is SteamBaseConn c && c != this && c.remoteId == id)) + { + ServerLog.Log($"Skipping delayed Steam P2P session close with {id}; a reconnect replaced it"); + return; + } + + CloseSteamSession(); + })); + } + + // Frees the underlying Steam P2P session. This is required so that a later reconnect from + // the same user produces a fresh P2PSessionRequest_t (and, on the host, a new accept prompt) + // instead of Steam silently reusing the still-open session, which left the peer stuck and the + // host without a prompt (#843). + // + // Called immediately when no goodbye is queued; server-initiated disconnects that queue a goodbye + // defer it (see OnClose) so SendP2PPacket can flush the reason before the session is torn down. + protected void CloseSteamSession() + { + ServerLog.Log($"Closing Steam P2P session with {remoteId}"); + SteamNetworking.CloseP2PSessionWithUser(remoteId); } public override string ToString() => $"SteamP2P ({remoteId}:{username})"; @@ -108,6 +158,10 @@ public override void OnKeepAliveArrived(bool idMatched) private void OnDisconnect() { + // The P2P timeout/error path does not go through OnClose, so close the Steam session here + // too. Otherwise the host keeps a half-open session with the departed client and their + // reconnect reuses it without firing a new accept prompt (#843). + CloseSteamSession(); serverPlayer.Server.playerManager.SetDisconnected(this, MpDisconnectReason.ClientLeft); } } @@ -132,10 +186,17 @@ public void Tick() var player = playerManager.Players .FirstOrDefault(p => p.conn is SteamBaseConn conn && conn.remoteId == packet.remote); - if (packet.joinPacket && player == null) + if (packet.joinPacket) { ConnectionBase conn = new SteamServerConn(packet.remote, packet.channel); + // A join packet from a remote we still consider connected means their previous + // session died and they are reconnecting on a fresh one (e.g. a quick rejoin before + // the old connection timed out). Without replacing the stale player the join would be + // discarded, leaving them stuck on "waiting for host to accept" (#843). + if (player != null) + playerManager.ReplaceStale(player, conn); + var preConnect = playerManager.OnPreConnect(packet.remote); if (preConnect != null) { @@ -155,14 +216,13 @@ public void Tick() conn.Send(Packets.Server_SteamAccept); } - else if (!packet.joinPacket && player != null) + else if (player != null) { player.HandleReceive(packet.data, packet.reliable); } else { - ServerLog.Error( - $"Received a join packet: {packet.joinPacket} for player: {player} (player should only be null when joinPacket is true)"); + ServerLog.Error($"Received a data packet from {packet.remote}, who has no connection"); } } } diff --git a/Source/Client/Networking/SteamIntegration.cs b/Source/Client/Networking/SteamIntegration.cs index 8bfa3f7cc..70d88cbef 100644 --- a/Source/Client/Networking/SteamIntegration.cs +++ b/Source/Client/Networking/SteamIntegration.cs @@ -34,23 +34,37 @@ public static void InitCallbacks() { ServerLog.Log($"Received P2P session request from {req.m_steamIDRemote}"); var session = Multiplayer.session; - if (Multiplayer.LocalServer?.settings.steam == true && !session.pendingSteam.Contains(req.m_steamIDRemote)) + if (Multiplayer.LocalServer?.settings.steam != true) + return; + + var remoteId = req.m_steamIDRemote; + + if (Multiplayer.settings.autoAcceptSteam) + { + SteamNetworking.AcceptP2PSessionWithUser(remoteId); + } + // pendingSteam doubles as the dedup set: an entry exists exactly while a prompt is + // unanswered (both accept and reject clear it), so this skips duplicate prompts + // without a stale entry ever blocking a reconnect permanently (#843). + else if (!session.pendingSteam.Contains(remoteId)) { - if (Multiplayer.settings.autoAcceptSteam) - SteamNetworking.AcceptP2PSessionWithUser(req.m_steamIDRemote); - else + session.pendingSteam.Add(remoteId); + PendingPlayerWindow.EnqueueJoinRequest(remoteId, (joinReq, accepted) => { - session.pendingSteam.Add(req.m_steamIDRemote); - PendingPlayerWindow.EnqueueJoinRequest(req.m_steamIDRemote, (joinReq, accepted) => - { - if(joinReq.steamId.HasValue && accepted) AcceptPlayerJoinRequest(joinReq.steamId.Value); - }); - } - session.knownUsers.Add(req.m_steamIDRemote); - session.NotifyChat(); - - SteamFriends.RequestUserInformation(req.m_steamIDRemote, true); + if (!joinReq.steamId.HasValue) return; + if (accepted) + AcceptPlayerJoinRequest(joinReq.steamId.Value); + else + // Clean up so the player isn't blocked from prompting again on reconnect. + session.pendingSteam.Remove(joinReq.steamId.Value); + }); } + + if (!session.knownUsers.Contains(remoteId)) + session.knownUsers.Add(remoteId); + session.NotifyChat(); + + SteamFriends.RequestUserInformation(remoteId, true); }); friendRchpUpdate = Callback.Create(update => diff --git a/Source/Common/Networking/ConnectionBase.cs b/Source/Common/Networking/ConnectionBase.cs index 14ab1b872..b5864d567 100644 --- a/Source/Common/Networking/ConnectionBase.cs +++ b/Source/Common/Networking/ConnectionBase.cs @@ -12,6 +12,8 @@ public abstract class ConnectionBase public virtual int Latency { get; set; } + public virtual object? RemoteIdentity => null; + public ConnectionStateEnum State { get; private set; } public MpConnectionState? StateObj { get; private set; } // If lenient is set, reliable packets without handlers are ignored instead of throwing an exception. @@ -269,6 +271,8 @@ public void Close(MpDisconnectReason reason, byte[]? data = null) OnClose(null); } + public virtual void CloseReplacedBy(ConnectionBase replacement, MpDisconnectReason reason) => Close(reason); + protected abstract void OnClose(ServerDisconnectPacket? goodbye); /// Invoked after a keep alive timer arrives. Only used by the server diff --git a/Source/Common/Networking/LiteNetConnection.cs b/Source/Common/Networking/LiteNetConnection.cs index b3a8c5d05..323f72cf2 100644 --- a/Source/Common/Networking/LiteNetConnection.cs +++ b/Source/Common/Networking/LiteNetConnection.cs @@ -7,6 +7,11 @@ public class LiteNetConnection(NetPeer peer) : ConnectionBase { public readonly NetPeer peer = peer; + // Only a hint at who the remote is: players behind one NAT share an address, so a housemate + // can match; and a player whose address changed (mobile, VPN, or v4 vs v6 across the two + // NetManagers) won't match their own earlier connection. + public override object? RemoteIdentity => peer.Address; + protected override void SendRaw(byte[] raw, bool reliable) { if (peer.ConnectionState == ConnectionState.Connected) diff --git a/Source/Common/Networking/State/ServerJoiningState.cs b/Source/Common/Networking/State/ServerJoiningState.cs index b2d2a392f..8d3b88e63 100644 --- a/Source/Common/Networking/State/ServerJoiningState.cs +++ b/Source/Common/Networking/State/ServerJoiningState.cs @@ -87,10 +87,19 @@ private void HandleUsername(ClientUsernamePacket packet) return; } - if (Server.GetPlayer(username) != null) + var existing = Server.GetPlayer(username); + if (existing != null) { - Player.Disconnect(MpDisconnectReason.UsernameAlreadyOnline); - return; + // Coming from the same remote as the player already holding this username means it's them + // reconnecting before their old connection was reaped, so give them their name back. Anyone + // else claiming it is turned away as before. + if (existing.conn.RemoteIdentity?.Equals(connection.RemoteIdentity) == true) + Server.playerManager.ReplaceStale(existing, connection); + else + { + Player.Disconnect(MpDisconnectReason.UsernameAlreadyOnline); + return; + } } connection.username = username; diff --git a/Source/Common/PlayerManager.cs b/Source/Common/PlayerManager.cs index efa871704..fb8d69fc3 100644 --- a/Source/Common/PlayerManager.cs +++ b/Source/Common/PlayerManager.cs @@ -62,6 +62,16 @@ public ServerPlayer OnConnected(ConnectionBase conn) return conn.serverPlayer; } + // Drops a connection that a newly arrived one has superseded, so the arrival can take its place + // instead of being turned away (#843). + public void ReplaceStale(ServerPlayer stale, ConnectionBase replacement) + { + ServerLog.Log($"Replacing stale connection {stale.conn} with {replacement}"); + + stale.conn.CloseReplacedBy(replacement, MpDisconnectReason.ClientLeft); + SetDisconnected(stale.conn, MpDisconnectReason.ClientLeft); + } + public void SetDisconnected(ConnectionBase conn, MpDisconnectReason reason) { if (conn.State == ConnectionStateEnum.Disconnected) return; diff --git a/Source/Tests/Helper/RecordingConnection.cs b/Source/Tests/Helper/RecordingConnection.cs index e219992a1..8708e39a2 100644 --- a/Source/Tests/Helper/RecordingConnection.cs +++ b/Source/Tests/Helper/RecordingConnection.cs @@ -15,6 +15,10 @@ public RecordingConnection(string username) public override int Latency { get => 0; set { } } + public object? remoteIdentity; + + public override object? RemoteIdentity => remoteIdentity; + protected override void SendRaw(byte[] raw, bool reliable) { if (raw.Length == 0) diff --git a/Source/Tests/Helper/TestUsernameOnlyState.cs b/Source/Tests/Helper/TestUsernameOnlyState.cs new file mode 100644 index 000000000..ca1bde85f --- /dev/null +++ b/Source/Tests/Helper/TestUsernameOnlyState.cs @@ -0,0 +1,27 @@ +using Multiplayer.Common; +using Multiplayer.Common.Networking.Packet; + +namespace Tests; + +// Gets as far as claiming a username and then stays put, so tests have a connection for a later +// client to collide with. +public class TestUsernameOnlyState : AsyncConnectionState +{ + public TestUsernameOnlyState(ConnectionBase connection) : base(connection) + { + } + + // Deliberately registers no packet handlers: handlers accumulate globally per connection state, so + // declaring one here would clash with the state a second client in the same test installs. + protected override async Task RunState() + { + connection.Send(ClientProtocolPacket.Current()); + await TypedPacket(); + + connection.Send(new ClientUsernamePacket(connection.username!)); + await TypedPacket(); + + // Left unanswered, which parks the connection here still holding the username. + await TypedPacket(); + } +} diff --git a/Source/Tests/ServerTest.cs b/Source/Tests/ServerTest.cs index 4cfff8bd5..bc1252e61 100644 --- a/Source/Tests/ServerTest.cs +++ b/Source/Tests/ServerTest.cs @@ -1,4 +1,5 @@ using System.Diagnostics; +using System.Net; using LiteNetLib; using Multiplayer.Common; @@ -125,6 +126,56 @@ public void StandaloneJoinWithExistingPlayer_DoesNotStartJoinPoint() Assert.That(server.worldData.CreatingJoinPoint, Is.False); } + [Test] + public void JoinFromSameAddress_ReplacesPlayerHoldingTheUsername() + { + var server = MakeServer(out var port); + var stalePlayer = AddPlayerHoldingTestUsername(server, IPAddress.Loopback); + + ConnectClient(port, typeof(TestUsernameOnlyState)); + + WaitUntil(() => !server.playerManager.Players.Contains(stalePlayer), + "the connection at the same address was not replaced"); + Assert.That(server.playerManager.GetPlayer("test1"), Is.Not.Null); + } + + [Test] + public void JoinFromDifferentAddress_KeepsPlayerHoldingTheUsername() + { + var server = MakeServer(out var port); + var existingPlayer = AddPlayerHoldingTestUsername(server, IPAddress.Parse("10.0.0.1")); + + ConnectClient(port, typeof(TestUsernameOnlyState)); + + // Nothing should displace them, so give the join time to go wrong before checking. + Thread.Sleep(500); + + Assert.That(server.playerManager.Players.Contains(existingPlayer), Is.True); + } + + // Stands in for a player whose client is gone but whose connection the server still holds. + private static ServerPlayer AddPlayerHoldingTestUsername(MultiplayerServer server, IPAddress address) + { + var conn = new RecordingConnection("test1") { remoteIdentity = address }; + conn.ChangeState(ConnectionStateEnum.ServerPlaying); + var player = new ServerPlayer(100, conn); + conn.serverPlayer = player; + server.playerManager.Players.Add(player); + return player; + } + + private static void WaitUntil(Func condition, string message) + { + var timeoutWatch = Stopwatch.StartNew(); + while (!condition()) + { + if (timeoutWatch.ElapsedMilliseconds > 2000) + Assert.Fail($"Timeout: {message}"); + + Thread.Sleep(50); + } + } + private void ConnectClient(int port, Type joiningStateType) { var clientListener = new TestNetListener(joiningStateType);