From 04cdc89a5c9f92c70078520eb5c63fd2606f2a22 Mon Sep 17 00:00:00 2001 From: Gary Wilber Date: Fri, 2 Oct 2020 17:14:57 -0700 Subject: [PATCH 1/4] Make MusicBrainzAlbumProvider thread safe --- .../MusicBrainz/MusicBrainzAlbumProvider.cs | 67 +++++++++++-------- 1 file changed, 39 insertions(+), 28 deletions(-) diff --git a/MediaBrowser.Providers/Plugins/MusicBrainz/MusicBrainzAlbumProvider.cs b/MediaBrowser.Providers/Plugins/MusicBrainz/MusicBrainzAlbumProvider.cs index abfa1c6e71..2d37422d04 100644 --- a/MediaBrowser.Providers/Plugins/MusicBrainz/MusicBrainzAlbumProvider.cs +++ b/MediaBrowser.Providers/Plugins/MusicBrainz/MusicBrainzAlbumProvider.cs @@ -46,6 +46,7 @@ namespace MediaBrowser.Providers.Music private readonly string _musicBrainzBaseUrl; + private SemaphoreSlim _apiRequestLock = new SemaphoreSlim(1, 1); private Stopwatch _stopWatchMusicBrainz = new Stopwatch(); public MusicBrainzAlbumProvider( @@ -742,45 +743,55 @@ namespace MediaBrowser.Providers.Music /// internal async Task GetMusicBrainzResponse(string url, CancellationToken cancellationToken) { - using var options = new HttpRequestMessage(HttpMethod.Get, _musicBrainzBaseUrl.TrimEnd('/') + url); - - // MusicBrainz request a contact email address is supplied, as comment, in user agent field: - // https://musicbrainz.org/doc/XML_Web_Service/Rate_Limiting#User-Agent - options.Headers.UserAgent.ParseAdd(string.Format( - CultureInfo.InvariantCulture, - "{0} ( {1} )", - _appHost.ApplicationUserAgent, - _appHost.ApplicationUserAgentAddress)); - HttpResponseMessage response; var attempts = 0u; + var requestUrl = _musicBrainzBaseUrl.TrimEnd('/') + url; - do + await _apiRequestLock.WaitAsync(cancellationToken).ConfigureAwait(false); + + try { - attempts++; - - if (_stopWatchMusicBrainz.ElapsedMilliseconds < _musicBrainzQueryIntervalMs) + do { - // MusicBrainz is extremely adamant about limiting to one request per second - var delayMs = _musicBrainzQueryIntervalMs - _stopWatchMusicBrainz.ElapsedMilliseconds; - await Task.Delay((int)delayMs, cancellationToken).ConfigureAwait(false); + attempts++; + + if (_stopWatchMusicBrainz.ElapsedMilliseconds < _musicBrainzQueryIntervalMs) + { + // MusicBrainz is extremely adamant about limiting to one request per second + var delayMs = _musicBrainzQueryIntervalMs - _stopWatchMusicBrainz.ElapsedMilliseconds; + await Task.Delay((int)delayMs, cancellationToken).ConfigureAwait(false); + } + + // Write time since last request to debug log as evidence we're meeting rate limit + // requirement, before resetting stopwatch back to zero. + _logger.LogDebug("GetMusicBrainzResponse: Time since previous request: {0} ms", _stopWatchMusicBrainz.ElapsedMilliseconds); + _stopWatchMusicBrainz.Restart(); + + using var request = new HttpRequestMessage(HttpMethod.Get, _musicBrainzBaseUrl.TrimEnd('/') + url); + + // MusicBrainz request a contact email address is supplied, as comment, in user agent field: + // https://musicbrainz.org/doc/XML_Web_Service/Rate_Limiting#User-Agent + request.Headers.UserAgent.ParseAdd(string.Format( + CultureInfo.InvariantCulture, + "{0} ( {1} )", + _appHost.ApplicationUserAgent, + _appHost.ApplicationUserAgentAddress)); + + response = await _httpClientFactory.CreateClient(NamedClient.Default).SendAsync(request).ConfigureAwait(false); + + // We retry a finite number of times, and only whilst MB is indicating 503 (throttling) } - - // Write time since last request to debug log as evidence we're meeting rate limit - // requirement, before resetting stopwatch back to zero. - _logger.LogDebug("GetMusicBrainzResponse: Time since previous request: {0} ms", _stopWatchMusicBrainz.ElapsedMilliseconds); - _stopWatchMusicBrainz.Restart(); - - response = await _httpClientFactory.CreateClient(NamedClient.Default).SendAsync(options).ConfigureAwait(false); - - // We retry a finite number of times, and only whilst MB is indicating 503 (throttling) + while (attempts < MusicBrainzQueryAttempts && response.StatusCode == HttpStatusCode.ServiceUnavailable); + } + finally + { + _apiRequestLock.Release(); } - while (attempts < MusicBrainzQueryAttempts && response.StatusCode == HttpStatusCode.ServiceUnavailable); // Log error if unable to query MB database due to throttling if (attempts == MusicBrainzQueryAttempts && response.StatusCode == HttpStatusCode.ServiceUnavailable) { - _logger.LogError("GetMusicBrainzResponse: 503 Service Unavailable (throttled) response received {0} times whilst requesting {1}", attempts, options.RequestUri); + _logger.LogError("GetMusicBrainzResponse: 503 Service Unavailable (throttled) response received {0} times whilst requesting {1}", attempts, requestUrl); } return response; From db2e667936a3c982ad28fb383efd427f24e4e37d Mon Sep 17 00:00:00 2001 From: Gary Wilber Date: Fri, 2 Oct 2020 17:19:35 -0700 Subject: [PATCH 2/4] expand try finally --- .../Plugins/MusicBrainz/MusicBrainzAlbumProvider.cs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/MediaBrowser.Providers/Plugins/MusicBrainz/MusicBrainzAlbumProvider.cs b/MediaBrowser.Providers/Plugins/MusicBrainz/MusicBrainzAlbumProvider.cs index 2d37422d04..5fe9ef7fa6 100644 --- a/MediaBrowser.Providers/Plugins/MusicBrainz/MusicBrainzAlbumProvider.cs +++ b/MediaBrowser.Providers/Plugins/MusicBrainz/MusicBrainzAlbumProvider.cs @@ -782,18 +782,18 @@ namespace MediaBrowser.Providers.Music // We retry a finite number of times, and only whilst MB is indicating 503 (throttling) } while (attempts < MusicBrainzQueryAttempts && response.StatusCode == HttpStatusCode.ServiceUnavailable); + + // Log error if unable to query MB database due to throttling + if (attempts == MusicBrainzQueryAttempts && response.StatusCode == HttpStatusCode.ServiceUnavailable) + { + _logger.LogError("GetMusicBrainzResponse: 503 Service Unavailable (throttled) response received {0} times whilst requesting {1}", attempts, requestUrl); + } } finally { _apiRequestLock.Release(); } - // Log error if unable to query MB database due to throttling - if (attempts == MusicBrainzQueryAttempts && response.StatusCode == HttpStatusCode.ServiceUnavailable) - { - _logger.LogError("GetMusicBrainzResponse: 503 Service Unavailable (throttled) response received {0} times whilst requesting {1}", attempts, requestUrl); - } - return response; } From 7841378506a04dd02d8e8459a274e740ed5da3f5 Mon Sep 17 00:00:00 2001 From: Gary Wilber Date: Fri, 2 Oct 2020 17:27:43 -0700 Subject: [PATCH 3/4] cleaner --- .../MusicBrainz/MusicBrainzAlbumProvider.cs | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/MediaBrowser.Providers/Plugins/MusicBrainz/MusicBrainzAlbumProvider.cs b/MediaBrowser.Providers/Plugins/MusicBrainz/MusicBrainzAlbumProvider.cs index 5fe9ef7fa6..bf3088aa83 100644 --- a/MediaBrowser.Providers/Plugins/MusicBrainz/MusicBrainzAlbumProvider.cs +++ b/MediaBrowser.Providers/Plugins/MusicBrainz/MusicBrainzAlbumProvider.cs @@ -743,14 +743,14 @@ namespace MediaBrowser.Providers.Music /// internal async Task GetMusicBrainzResponse(string url, CancellationToken cancellationToken) { - HttpResponseMessage response; - var attempts = 0u; - var requestUrl = _musicBrainzBaseUrl.TrimEnd('/') + url; - await _apiRequestLock.WaitAsync(cancellationToken).ConfigureAwait(false); try { + HttpResponseMessage response; + var attempts = 0u; + var requestUrl = _musicBrainzBaseUrl.TrimEnd('/') + url; + do { attempts++; @@ -767,7 +767,7 @@ namespace MediaBrowser.Providers.Music _logger.LogDebug("GetMusicBrainzResponse: Time since previous request: {0} ms", _stopWatchMusicBrainz.ElapsedMilliseconds); _stopWatchMusicBrainz.Restart(); - using var request = new HttpRequestMessage(HttpMethod.Get, _musicBrainzBaseUrl.TrimEnd('/') + url); + using var request = new HttpRequestMessage(HttpMethod.Get, requestUrl); // MusicBrainz request a contact email address is supplied, as comment, in user agent field: // https://musicbrainz.org/doc/XML_Web_Service/Rate_Limiting#User-Agent @@ -788,13 +788,13 @@ namespace MediaBrowser.Providers.Music { _logger.LogError("GetMusicBrainzResponse: 503 Service Unavailable (throttled) response received {0} times whilst requesting {1}", attempts, requestUrl); } + + return response; } finally { _apiRequestLock.Release(); } - - return response; } /// From b1f637684dfd3511c2f6be643efa5ff7cecadb17 Mon Sep 17 00:00:00 2001 From: Gary Wilber Date: Fri, 16 Oct 2020 11:15:42 -0700 Subject: [PATCH 4/4] Apply suggestions from code review (updating comments) Co-authored-by: BaronGreenback --- .../Plugins/MusicBrainz/MusicBrainzAlbumProvider.cs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/MediaBrowser.Providers/Plugins/MusicBrainz/MusicBrainzAlbumProvider.cs b/MediaBrowser.Providers/Plugins/MusicBrainz/MusicBrainzAlbumProvider.cs index bf3088aa83..31f0123dcf 100644 --- a/MediaBrowser.Providers/Plugins/MusicBrainz/MusicBrainzAlbumProvider.cs +++ b/MediaBrowser.Providers/Plugins/MusicBrainz/MusicBrainzAlbumProvider.cs @@ -757,7 +757,7 @@ namespace MediaBrowser.Providers.Music if (_stopWatchMusicBrainz.ElapsedMilliseconds < _musicBrainzQueryIntervalMs) { - // MusicBrainz is extremely adamant about limiting to one request per second + // MusicBrainz is extremely adamant about limiting to one request per second. var delayMs = _musicBrainzQueryIntervalMs - _stopWatchMusicBrainz.ElapsedMilliseconds; await Task.Delay((int)delayMs, cancellationToken).ConfigureAwait(false); } @@ -770,7 +770,7 @@ namespace MediaBrowser.Providers.Music using var request = new HttpRequestMessage(HttpMethod.Get, requestUrl); // MusicBrainz request a contact email address is supplied, as comment, in user agent field: - // https://musicbrainz.org/doc/XML_Web_Service/Rate_Limiting#User-Agent + // https://musicbrainz.org/doc/XML_Web_Service/Rate_Limiting#User-Agent . request.Headers.UserAgent.ParseAdd(string.Format( CultureInfo.InvariantCulture, "{0} ( {1} )", @@ -779,11 +779,11 @@ namespace MediaBrowser.Providers.Music response = await _httpClientFactory.CreateClient(NamedClient.Default).SendAsync(request).ConfigureAwait(false); - // We retry a finite number of times, and only whilst MB is indicating 503 (throttling) + // We retry a finite number of times, and only whilst MB is indicating 503 (throttling). } while (attempts < MusicBrainzQueryAttempts && response.StatusCode == HttpStatusCode.ServiceUnavailable); - // Log error if unable to query MB database due to throttling + // Log error if unable to query MB database due to throttling. if (attempts == MusicBrainzQueryAttempts && response.StatusCode == HttpStatusCode.ServiceUnavailable) { _logger.LogError("GetMusicBrainzResponse: 503 Service Unavailable (throttled) response received {0} times whilst requesting {1}", attempts, requestUrl);