From 8e41e48985b6dcf3ea22e424d8af12d84f4e32a1 Mon Sep 17 00:00:00 2001 From: da3dsoul Date: Sun, 10 Nov 2024 13:44:45 -0500 Subject: [PATCH] Improve locking of the AniDB Socket Handler --- .../Providers/AniDB/UDP/AniDBSocketHandler.cs | 116 ++++++------ .../AniDB/UDP/AniDBUDPConnectionHandler.cs | 175 +++++++++++------- 2 files changed, 170 insertions(+), 121 deletions(-) diff --git a/Shoko.Server/Providers/AniDB/UDP/AniDBSocketHandler.cs b/Shoko.Server/Providers/AniDB/UDP/AniDBSocketHandler.cs index 95cc59ca2..1755120a0 100644 --- a/Shoko.Server/Providers/AniDB/UDP/AniDBSocketHandler.cs +++ b/Shoko.Server/Providers/AniDB/UDP/AniDBSocketHandler.cs @@ -110,67 +110,71 @@ private void EmptyBuffer() public async Task TryConnection() { if (IsConnected) return true; - await _semaphore.WaitAsync(); - // Don't send Expect 100 requests. These requests aren't always supported by remote internet devices, in which case can cause failure. - ServicePointManager.Expect100Continue = false; - try { - _localIpEndPoint = new IPEndPoint(IPAddress.Any, _clientPort); - - // we use bind() here (normally only for servers, not clients) instead of connect() because of this: - /* - * Local Port - * A client should select a fixed local port >1024 at install time and reuse it for local UDP Sockets. If the API sees too many different UDP Ports from one IP within ~1 hour it will ban the IP. (So make sure you're reusing your UDP ports also for testing/debugging!) - * The local port may be hardcoded, however, an option to manually specify another port should be offered. - */ - _aniDBSocket.Bind(_localIpEndPoint); - _aniDBSocket.ReceiveTimeout = ReceiveTimeoutMs; - - _logger.LogInformation("Bound to local address: {Local} - Port: {ClientPort} ({Family})", _localIpEndPoint, - _clientPort, _localIpEndPoint.AddressFamily); - } - catch (Exception ex) - { - _logger.LogError(ex, "Could not bind to local port"); - _semaphore.Release(); - IsConnected = false; - return false; - } + await _semaphore.WaitAsync(); + // Don't send Expect 100 requests. These requests aren't always supported by remote internet devices, in which case can cause failure. + ServicePointManager.Expect100Continue = false; - try - { - var remoteHostEntry = await Dns.GetHostEntryAsync(_serverHost).WaitAsync(TimeSpan.FromSeconds(5)).ConfigureAwait(false); - _remoteIpEndPoint = new IPEndPoint(remoteHostEntry.AddressList[0], _serverPort); + try + { + _localIpEndPoint = new IPEndPoint(IPAddress.Any, _clientPort); + + // we use bind() here (normally only for servers, not clients) instead of connect() because of this: + /* + * Local Port + * A client should select a fixed local port >1024 at install time and reuse it for local UDP Sockets. If the API sees too many different UDP Ports from one IP within ~1 hour it will ban the IP. (So make sure you're reusing your UDP ports also for testing/debugging!) + * The local port may be hardcoded, however, an option to manually specify another port should be offered. + */ + _aniDBSocket.Bind(_localIpEndPoint); + _aniDBSocket.ReceiveTimeout = ReceiveTimeoutMs; + + _logger.LogInformation("Bound to local address: {Local} - Port: {ClientPort} ({Family})", _localIpEndPoint, + _clientPort, _localIpEndPoint.AddressFamily); + } + catch (Exception ex) + { + _logger.LogError(ex, "Could not bind to local port"); + IsConnected = false; + return false; + } + + try + { + var remoteHostEntry = await Dns.GetHostEntryAsync(_serverHost).WaitAsync(TimeSpan.FromSeconds(5)).ConfigureAwait(false); + _remoteIpEndPoint = new IPEndPoint(remoteHostEntry.AddressList[0], _serverPort); - _logger.LogInformation("Bound to remote address: {Address} : {Port}", _remoteIpEndPoint.Address, - _remoteIpEndPoint.Port); + _logger.LogInformation("Bound to remote address: {Address} : {Port}", _remoteIpEndPoint.Address, + _remoteIpEndPoint.Port); + } + catch (Exception ex) + { + _logger.LogError(ex, "Could not bind to remote port"); + IsConnected = false; + return false; + } + + IsConnected = true; + return true; } - catch (Exception ex) + finally { - _logger.LogError(ex, "Could not bind to remote port"); _semaphore.Release(); - IsConnected = false; - return false; } - - _semaphore.Release(); - IsConnected = true; - return true; } public async ValueTask DisposeAsync() { await _semaphore.WaitAsync(); GC.SuppressFinalize(this); - if (_aniDBSocket == null) - { - IsConnected = false; - return; - } - try { + if (_aniDBSocket == null) + { + IsConnected = false; + return; + } + if (_aniDBSocket.Connected) { _aniDBSocket.Shutdown(SocketShutdown.Both); @@ -181,6 +185,9 @@ public async ValueTask DisposeAsync() { await _aniDBSocket.DisconnectAsync(false); } + + _aniDBSocket.Close(); + _logger.LogInformation("Closed AniDB Connection"); } catch (SocketException ex) { @@ -188,8 +195,6 @@ public async ValueTask DisposeAsync() } finally { - _aniDBSocket.Close(); - _logger.LogInformation("Closed AniDB Connection"); _semaphore.Release(); _semaphore.Dispose(); IsConnected = false; @@ -200,14 +205,14 @@ public void Dispose() { _semaphore.Wait(); GC.SuppressFinalize(this); - if (_aniDBSocket == null) - { - IsConnected = false; - return; - } - try { + if (_aniDBSocket == null) + { + IsConnected = false; + return; + } + if (_aniDBSocket.Connected) { _aniDBSocket.Shutdown(SocketShutdown.Both); @@ -218,6 +223,9 @@ public void Dispose() { _aniDBSocket.Disconnect(false); } + + _aniDBSocket.Close(); + _logger.LogInformation("Closed AniDB Connection"); } catch (SocketException ex) { @@ -225,8 +233,6 @@ public void Dispose() } finally { - _aniDBSocket.Close(); - _logger.LogInformation("Closed AniDB Connection"); _semaphore.Release(); _semaphore.Dispose(); IsConnected = false; diff --git a/Shoko.Server/Providers/AniDB/UDP/AniDBUDPConnectionHandler.cs b/Shoko.Server/Providers/AniDB/UDP/AniDBUDPConnectionHandler.cs index a18f0e00e..b95286709 100644 --- a/Shoko.Server/Providers/AniDB/UDP/AniDBUDPConnectionHandler.cs +++ b/Shoko.Server/Providers/AniDB/UDP/AniDBUDPConnectionHandler.cs @@ -3,6 +3,7 @@ using System.Net.Sockets; using System.Text; using System.Text.RegularExpressions; +using System.Threading; using System.Threading.Tasks; using System.Timers; using Microsoft.Extensions.Logging; @@ -34,7 +35,8 @@ public class AniDBUDPConnectionHandler : ConnectionHandler, IUDPConnectionHandle private const int LogoutPeriod = 5 * 60 * 1000; private readonly IRequestFactory _requestFactory; private readonly IConnectivityService _connectivityService; - private IAniDBSocketHandler? _socketHandler; + private AniDBSocketHandler? _socketHandler; + private readonly SemaphoreSlim _socketHandlerLock = new(1, 1); private static readonly Regex s_logMask = new("(?<=(\\bpass=|&pass=\\bs=|&s=))[^&]+", RegexOptions.Compiled | RegexOptions.IgnoreCase); public event EventHandler? LoginFailed; @@ -136,21 +138,29 @@ public async Task Init(string username, string password, string serverName private async Task InitInternal() { - if (_socketHandler != null) - { - await _socketHandler.DisposeAsync(); - _socketHandler = null; - } - var settings = SettingsProvider.GetSettings(); ArgumentNullException.ThrowIfNull(settings.AniDb?.UDPServerAddress); if (settings.AniDb.UDPServerPort == 0) throw new ArgumentException("AniDB UDP Server Port is invalid"); if (settings.AniDb.ClientPort == 0) throw new ArgumentException("AniDB Client Port is invalid"); - _socketHandler = new AniDBSocketHandler(_loggerFactory, settings.AniDb.UDPServerAddress, settings.AniDb.UDPServerPort, - settings.AniDb.ClientPort); - _isLoggedOn = false; - IsNetworkAvailable = await _socketHandler.TryConnection(); + try + { + await _socketHandlerLock.WaitAsync(); + if (_socketHandler != null) + { + await _socketHandler.DisposeAsync(); + _socketHandler = null; + } + + _socketHandler = new AniDBSocketHandler(_loggerFactory, settings.AniDb.UDPServerAddress, settings.AniDb.UDPServerPort, settings.AniDb.ClientPort); + IsNetworkAvailable = await _socketHandler.TryConnection(); + } + finally + { + _socketHandlerLock.Release(); + } + + _isLoggedOn = false; _pingTimer = new Timer { Interval = settings.AniDb.UDPPingFrequency * 1000, Enabled = true, AutoReset = true }; _pingTimer.Elapsed += PingTimerElapsed; _logoutTimer = new Timer { Interval = LogoutPeriod, Enabled = true, AutoReset = false }; @@ -164,7 +174,16 @@ private void PingTimerElapsed(object? sender, ElapsedEventArgs e) try { if (!_isLoggedOn) return; - if (_socketHandler == null || _socketHandler.IsLocked || !_socketHandler.IsConnected) return; + _socketHandlerLock.Wait(); + try + { + if (_socketHandler == null || _socketHandler.IsLocked || !_socketHandler.IsConnected) return; + } + finally + { + _socketHandlerLock.Release(); + } + if (IsBanned || BackoffSecs.HasValue) return; var ping = _requestFactory.Create(); @@ -189,10 +208,18 @@ private void LogoutTimerElapsed(object? sender, ElapsedEventArgs e) try { if (!_isLoggedOn) return; - if (_socketHandler == null || _socketHandler.IsLocked || !_socketHandler.IsConnected) return; - if (IsBanned || BackoffSecs.HasValue) return; + _socketHandlerLock.Wait(); + try + { + if (_socketHandler == null || _socketHandler.IsLocked || !_socketHandler.IsConnected) return; + if (IsBanned || BackoffSecs.HasValue) return; - ForceLogout(); + ForceLogout(); + } + finally + { + _socketHandlerLock.Release(); + } } catch (Exception exception) { @@ -268,62 +295,71 @@ public Task SendDirectly(string command, bool needsUnicode = true, bool private async Task SendInternal(string command, bool needsUnicode = true, bool isPing = false) { - ObjectDisposedException.ThrowIf(_socketHandler is not { IsConnected: true }, "The connection was closed by shoko"); - - // 1. Call AniDB - // 2. Decode the response, converting Unicode and decompressing, as needed - // 3. Check for an Error Response - // 4. Return a pretty response object, with a parsed return code and trimmed string - var encoding = needsUnicode ? new UnicodeEncoding(true, false) : Encoding.ASCII; - var sendByteAdd = encoding.GetBytes(command); - var timeoutPolicy = Policy - .Handle(e => e is { SocketErrorCode: SocketError.TimedOut }) - .Or() - .RetryAsync(async (_, _) => - { - Logger.LogWarning("AniDB request timed out. Checking Network and trying again"); - await _connectivityService.CheckAvailability(); - }); - var result = await timeoutPolicy.ExecuteAndCaptureAsync(async () => await RateLimiter.EnsureRateAsync(forceShortDelay: isPing, action: async () => + try { - if (_connectivityService.NetworkAvailability < NetworkAvailability.PartialInternet) + await _socketHandlerLock.WaitAsync(); + ObjectDisposedException.ThrowIf(_socketHandler is not { IsConnected: true }, "The connection was closed by shoko"); + + // 1. Call AniDB + // 2. Decode the response, converting Unicode and decompressing, as needed + // 3. Check for an Error Response + // 4. Return a pretty response object, with a parsed return code and trimmed string + var encoding = needsUnicode ? new UnicodeEncoding(true, false) : Encoding.ASCII; + var sendByteAdd = encoding.GetBytes(command); + var timeoutPolicy = Policy + .Handle(e => e is { SocketErrorCode: SocketError.TimedOut }) + .Or() + .RetryAsync(async (_, _) => + { + Logger.LogWarning("AniDB request timed out. Checking Network and trying again"); + await _connectivityService.CheckAvailability(); + }); + + var result = await timeoutPolicy.ExecuteAndCaptureAsync(async () => await RateLimiter.EnsureRateAsync(forceShortDelay: isPing, action: async () => { - Logger.LogError("No internet, so not sending AniDB request"); - throw new SocketException((int)SocketError.HostUnreachable); - } + if (_connectivityService.NetworkAvailability < NetworkAvailability.PartialInternet) + { + Logger.LogError("No internet, so not sending AniDB request"); + throw new SocketException((int)SocketError.HostUnreachable); + } - var start = DateTime.Now; - Logger.LogTrace("AniDB UDP Call: (Using {Unicode}) {Command}", needsUnicode ? "Unicode" : "ASCII", MaskLog(command)); - var byReceivedAdd = await _socketHandler.Send(sendByteAdd); + var start = DateTime.Now; + Logger.LogTrace("AniDB UDP Call: (Using {Unicode}) {Command}", needsUnicode ? "Unicode" : "ASCII", MaskLog(command)); + var byReceivedAdd = await _socketHandler.Send(sendByteAdd); - if (byReceivedAdd.All(a => a == 0)) - { - // we are probably banned or have lost connection. We can't tell the difference, so we're assuming ban - IsBanned = true; - throw new AniDBBannedException + if (byReceivedAdd.All(a => a == 0)) { - BanType = UpdateType.UDPBan, - BanExpires = BanTime?.AddHours(BanTimerResetLength) - }; - } + // we are probably banned or have lost connection. We can't tell the difference, so we're assuming ban + IsBanned = true; + throw new AniDBBannedException + { + BanType = UpdateType.UDPBan, + BanExpires = BanTime?.AddHours(BanTimerResetLength) + }; + } - // decode - var decodedString = Utils.GetEncoding(byReceivedAdd).GetString(byReceivedAdd, 0, byReceivedAdd.Length); - // remove BOM - if (decodedString[0] == 0xFEFF) decodedString = decodedString[1..]; + // decode + var decodedString = Utils.GetEncoding(byReceivedAdd).GetString(byReceivedAdd, 0, byReceivedAdd.Length); + // remove BOM + if (decodedString[0] == 0xFEFF) decodedString = decodedString[1..]; - var ts = DateTime.Now - start; - Logger.LogTrace("AniDB Response: Received in {Time:ss'.'ffff}s\n{DecodedString}", ts, MaskLog(decodedString)); - return decodedString; - })); + var ts = DateTime.Now - start; + Logger.LogTrace("AniDB Response: Received in {Time:ss'.'ffff}s\n{DecodedString}", ts, MaskLog(decodedString)); + return decodedString; + })); - if (result.FinalException != null) + if (result.FinalException != null) + { + Logger.LogError(result.FinalException, "Failed to send AniDB message"); + throw result.FinalException; + } + + return result.Result; + } + finally { - Logger.LogError(result.FinalException, "Failed to send AniDB message"); - throw result.FinalException; + _socketHandlerLock.Release(); } - - return result.Result; } private void StopPinging() @@ -409,11 +445,18 @@ public async Task CloseConnections() _logoutTimer?.Dispose(); _logoutTimer = null; - if (_socketHandler == null) return; - - Logger.LogInformation("AniDB UDP Socket Disposing..."); - await _socketHandler.DisposeAsync(); - _socketHandler = null; + await _socketHandlerLock.WaitAsync(); + try + { + if (_socketHandler == null) return; + Logger.LogInformation("AniDB UDP Socket Disposing..."); + await _socketHandler.DisposeAsync(); + _socketHandler = null; + } + finally + { + _socketHandlerLock.Release(); + } } public async Task Login()