From f1362ee0474f774375ce36efa06de0a69ae7c61a Mon Sep 17 00:00:00 2001 From: Christopher F Date: Sun, 7 Jan 2018 22:33:11 -0500 Subject: [PATCH 1/2] Log a fatal exception when the WebSocket cannot be recovered This resolves #883. The previous behavior when the WebSocket was closed with a 4006 (invalid state) was to kill the reconnect task and log an exception. While this behavior is 'correct', it does not make it obvious to the user (or developer, in my case) as to why their client has just quit and not bothered to attempt a reconnect. The current best way to determine if the client needs to be reset is to check that CloseCode property on the WebSocketException is set to 4006. However, since Discord does not have close code 4006 documented, it feels like only half of a solution. This change allows users to opt-out of fatal invalid-states in the client's configuration - though it is discouraged. If they have not opted out of them, a 4006 will trigger a FatalException, which can then be caught with a more simple pattern match in the Log event. This change also expands what we consider to be a fatal close code, and will raise a FatalException for a 1000 (Discord should never close a connection for us), 4004 (invalid token), 4010/4011 (shard related). --- src/Discord.Net.Core/Net/FatalException.cs | 12 ++++++++++++ .../Net/WebSocketClosedException.cs | 5 +++-- src/Discord.Net.WebSocket/Audio/AudioClient.cs | 4 ++-- src/Discord.Net.WebSocket/ConnectionManager.cs | 16 ++++++++++------ .../DiscordSocketApiClient.cs | 2 +- src/Discord.Net.WebSocket/DiscordSocketClient.cs | 4 ++-- src/Discord.Net.WebSocket/DiscordSocketConfig.cs | 6 +++++- 7 files changed, 35 insertions(+), 14 deletions(-) create mode 100644 src/Discord.Net.Core/Net/FatalException.cs diff --git a/src/Discord.Net.Core/Net/FatalException.cs b/src/Discord.Net.Core/Net/FatalException.cs new file mode 100644 index 000000000..29404b8a0 --- /dev/null +++ b/src/Discord.Net.Core/Net/FatalException.cs @@ -0,0 +1,12 @@ +using System; + +namespace Discord.Net +{ + public class FatalException : Exception + { + public FatalException(string reason, Exception inner) + : base(reason, inner) + { + } + } +} diff --git a/src/Discord.Net.Core/Net/WebSocketClosedException.cs b/src/Discord.Net.Core/Net/WebSocketClosedException.cs index d647b6c8c..119535974 100644 --- a/src/Discord.Net.Core/Net/WebSocketClosedException.cs +++ b/src/Discord.Net.Core/Net/WebSocketClosedException.cs @@ -1,4 +1,5 @@ -using System; +using System; + namespace Discord.Net { public class WebSocketClosedException : Exception @@ -6,7 +7,7 @@ namespace Discord.Net public int CloseCode { get; } public string Reason { get; } - public WebSocketClosedException(int closeCode, string reason = null) + public WebSocketClosedException(int closeCode, string reason = null, bool fatal = false) : base($"The server sent close {closeCode}{(reason != null ? $": \"{reason}\"" : "")}") { CloseCode = closeCode; diff --git a/src/Discord.Net.WebSocket/Audio/AudioClient.cs b/src/Discord.Net.WebSocket/Audio/AudioClient.cs index 1f33b3cc5..d801fc818 100644 --- a/src/Discord.Net.WebSocket/Audio/AudioClient.cs +++ b/src/Discord.Net.WebSocket/Audio/AudioClient.cs @@ -1,4 +1,4 @@ -using Discord.API.Voice; +using Discord.API.Voice; using Discord.Audio.Streams; using Discord.Logging; using Discord.Net.Converters; @@ -71,7 +71,7 @@ namespace Discord.Audio ApiClient.ReceivedPacket += ProcessPacketAsync; _stateLock = new SemaphoreSlim(1, 1); - _connection = new ConnectionManager(_stateLock, _audioLogger, 30000, + _connection = new ConnectionManager(_stateLock, _audioLogger, 30000, false, OnConnectingAsync, OnDisconnectingAsync, x => ApiClient.Disconnected += x); _connection.Connected += () => _connectedEvent.InvokeAsync(); _connection.Disconnected += (ex, recon) => _disconnectedEvent.InvokeAsync(ex); diff --git a/src/Discord.Net.WebSocket/ConnectionManager.cs b/src/Discord.Net.WebSocket/ConnectionManager.cs index decae4163..038efec26 100644 --- a/src/Discord.Net.WebSocket/ConnectionManager.cs +++ b/src/Discord.Net.WebSocket/ConnectionManager.cs @@ -1,13 +1,17 @@ using Discord.Logging; +using Discord.Net; using System; using System.Threading; using System.Threading.Tasks; -using Discord.Net; +using System.Linq; namespace Discord { internal class ConnectionManager { + // close codes that cannot be recovered from + private static readonly int[] _fatalErrorCodes = { 4004, 4010, 4011 }; + public event Func Connected { add { _connectedEvent.Add(value); } remove { _connectedEvent.Remove(value); } } private readonly AsyncEvent> _connectedEvent = new AsyncEvent>(); public event Func Disconnected { add { _disconnectedEvent.Add(value); } remove { _disconnectedEvent.Remove(value); } } @@ -26,7 +30,7 @@ namespace Discord public ConnectionState State { get; private set; } public CancellationToken CancelToken { get; private set; } - internal ConnectionManager(SemaphoreSlim stateLock, Logger logger, int connectionTimeout, + internal ConnectionManager(SemaphoreSlim stateLock, Logger logger, int connectionTimeout, bool invalidStateFatal, Func onConnecting, Func onDisconnecting, Action> clientDisconnectHandler) { _stateLock = stateLock; @@ -40,8 +44,8 @@ namespace Discord if (ex != null) { var ex2 = ex as WebSocketClosedException; - if (ex2?.CloseCode == 4006) - CriticalError(new Exception("WebSocket session expired", ex)); + if ((invalidStateFatal && ex2?.CloseCode == 4006) || _fatalErrorCodes.Contains(ex2?.CloseCode ?? 0)) + FatalError(new FatalException("WebSocket connection was closed with an unrecoverable error", ex)); else Error(new Exception("WebSocket connection was closed", ex)); } @@ -186,7 +190,7 @@ namespace Discord _connectionPromise.TrySetException(ex); _connectionCancelToken?.Cancel(); } - public void CriticalError(Exception ex) + public void FatalError(Exception ex) { _reconnectCancelToken?.Cancel(); Error(ex); @@ -207,4 +211,4 @@ namespace Discord } } } -} \ No newline at end of file +} diff --git a/src/Discord.Net.WebSocket/DiscordSocketApiClient.cs b/src/Discord.Net.WebSocket/DiscordSocketApiClient.cs index 72781204c..f2f1a5113 100644 --- a/src/Discord.Net.WebSocket/DiscordSocketApiClient.cs +++ b/src/Discord.Net.WebSocket/DiscordSocketApiClient.cs @@ -1,4 +1,4 @@ -#pragma warning disable CS1591 +#pragma warning disable CS1591 using Discord.API.Gateway; using Discord.API.Rest; using Discord.Net.Queue; diff --git a/src/Discord.Net.WebSocket/DiscordSocketClient.cs b/src/Discord.Net.WebSocket/DiscordSocketClient.cs index cb3f23c57..b943ba9bc 100644 --- a/src/Discord.Net.WebSocket/DiscordSocketClient.cs +++ b/src/Discord.Net.WebSocket/DiscordSocketClient.cs @@ -90,7 +90,7 @@ namespace Discord.WebSocket _stateLock = new SemaphoreSlim(1, 1); _gatewayLogger = LogManager.CreateLogger(ShardId == 0 && TotalShards == 1 ? "Gateway" : $"Shard #{ShardId}"); - _connection = new ConnectionManager(_stateLock, _gatewayLogger, config.ConnectionTimeout, + _connection = new ConnectionManager(_stateLock, _gatewayLogger, config.ConnectionTimeout, config.InvalidStateFatal, OnConnectingAsync, OnDisconnectingAsync, x => ApiClient.Disconnected += x); _connection.Connected += () => TimedInvokeAsync(_connectedEvent, nameof(Connected)); _connection.Disconnected += (ex, recon) => TimedInvokeAsync(_disconnectedEvent, nameof(Disconnected), ex); @@ -466,7 +466,7 @@ namespace Discord.WebSocket } catch (Exception ex) { - _connection.CriticalError(new Exception("Processing READY failed", ex)); + _connection.FatalError(new Exception("Processing READY failed", ex)); return; } diff --git a/src/Discord.Net.WebSocket/DiscordSocketConfig.cs b/src/Discord.Net.WebSocket/DiscordSocketConfig.cs index 3f9c18863..542efbfc3 100644 --- a/src/Discord.Net.WebSocket/DiscordSocketConfig.cs +++ b/src/Discord.Net.WebSocket/DiscordSocketConfig.cs @@ -1,4 +1,4 @@ -using Discord.Net.Udp; +using Discord.Net.Udp; using Discord.Net.WebSockets; using Discord.Rest; @@ -25,6 +25,10 @@ namespace Discord.WebSocket /// Gets or sets the max number of users a guild may have for offline users to be included in the READY packet. Max is 250. /// public int LargeThreshold { get; set; } = 250; + /// + /// Gets or sets whether a State Invalidation from Discord should be fatal. Setting this to false could lead to unexpected behavior when Discord is unstable. + /// + public bool InvalidStateFatal { get; set; } = true; /// Gets or sets the provider used to generate new websocket connections. public WebSocketProvider WebSocketProvider { get; set; } From 2041e475ddfaab26e9c7d26ed56dede51c5a0209 Mon Sep 17 00:00:00 2001 From: Christopher F Date: Sun, 7 Jan 2018 23:43:31 -0500 Subject: [PATCH 2/2] Remove unused 'fatal' bool in WebSocketClosedException#ctor --- src/Discord.Net.Core/Net/WebSocketClosedException.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Discord.Net.Core/Net/WebSocketClosedException.cs b/src/Discord.Net.Core/Net/WebSocketClosedException.cs index 119535974..989fad20d 100644 --- a/src/Discord.Net.Core/Net/WebSocketClosedException.cs +++ b/src/Discord.Net.Core/Net/WebSocketClosedException.cs @@ -7,7 +7,7 @@ namespace Discord.Net public int CloseCode { get; } public string Reason { get; } - public WebSocketClosedException(int closeCode, string reason = null, bool fatal = false) + public WebSocketClosedException(int closeCode, string reason = null) : base($"The server sent close {closeCode}{(reason != null ? $": \"{reason}\"" : "")}") { CloseCode = closeCode;