Move MusicBrainz Query client to plugin instance

3 files changed, 116 insertions, 138 deletions

diff --git a/MediaBrowser.Providers/Plugins/MusicBrainz/MusicBrainzAlbumProvider.cs b/MediaBrowser.Providers/Plugins/MusicBrainz/MusicBrainzAlbumProvider.cs
index 88c8e4f7c9..715bdd9da4 100644
--- a/MediaBrowser.Providers/Plugins/MusicBrainz/MusicBrainzAlbumProvider.cs
+++ b/MediaBrowser.Providers/Plugins/MusicBrainz/MusicBrainzAlbumProvider.cs
@@ -9,81 +9,41 @@ using Jellyfin.Extensions;
 using MediaBrowser.Controller.Entities.Audio;
 using MediaBrowser.Controller.Providers;
 using MediaBrowser.Model.Entities;
-using MediaBrowser.Model.Plugins;
 using MediaBrowser.Model.Providers;
 using MediaBrowser.Providers.Music;
-using MediaBrowser.Providers.Plugins.MusicBrainz.Configuration;
 using MetaBrainz.MusicBrainz;
 using MetaBrainz.MusicBrainz.Interfaces.Entities;
 using MetaBrainz.MusicBrainz.Interfaces.Searches;
-using Microsoft.Extensions.Logging;
 
 namespace MediaBrowser.Providers.Plugins.MusicBrainz;
 
 /// <summary>
 /// Music album metadata provider for MusicBrainz.
 /// </summary>
-public class MusicBrainzAlbumProvider : IRemoteMetadataProvider<MusicAlbum, AlbumInfo>, IHasOrder, IDisposable
+public class MusicBrainzAlbumProvider : IRemoteMetadataProvider<MusicAlbum, AlbumInfo>, IHasOrder
 {
-    private readonly ILogger<MusicBrainzAlbumProvider> _logger;
-    private Query _musicBrainzQuery;
-
-    /// <summary>
-    /// Initializes a new instance of the <see cref="MusicBrainzAlbumProvider"/> class.
-    /// </summary>
-    /// <param name="logger">The logger.</param>
-    public MusicBrainzAlbumProvider(ILogger<MusicBrainzAlbumProvider> logger)
-    {
-        _logger = logger;
-        _musicBrainzQuery = new Query();
-        ReloadConfig(null, MusicBrainz.Plugin.Instance!.Configuration);
-        MusicBrainz.Plugin.Instance!.ConfigurationChanged += ReloadConfig;
-    }
-
     /// <inheritdoc />
     public string Name => "MusicBrainz";
 
     /// <inheritdoc />
     public int Order => 0;
 
-    private void ReloadConfig(object? sender, BasePluginConfiguration e)
-    {
-        var configuration = (PluginConfiguration)e;
-        if (Uri.TryCreate(configuration.Server, UriKind.Absolute, out var server))
-        {
-            Query.DefaultServer = server.DnsSafeHost;
-            Query.DefaultPort = server.Port;
-            Query.DefaultUrlScheme = server.Scheme;
-        }
-        else
-        {
-            // Fallback to official server
-            _logger.LogWarning("Invalid MusicBrainz server specified, falling back to official server");
-            var defaultServer = new Uri(PluginConfiguration.DefaultServer);
-            Query.DefaultServer = defaultServer.Host;
-            Query.DefaultPort = defaultServer.Port;
-            Query.DefaultUrlScheme = defaultServer.Scheme;
-        }
-
-        Query.DelayBetweenRequests = configuration.RateLimit;
-        _musicBrainzQuery = new Query();
-    }
-
     /// <inheritdoc />
     public async Task<IEnumerable<RemoteSearchResult>> GetSearchResults(AlbumInfo searchInfo, CancellationToken cancellationToken)
     {
+        var query = MusicBrainz.Plugin.Instance!.MusicBrainzQuery;
         var releaseId = searchInfo.GetReleaseId();
         var releaseGroupId = searchInfo.GetReleaseGroupId();
 
         if (!string.IsNullOrEmpty(releaseId))
         {
-            var releaseResult = await _musicBrainzQuery.LookupReleaseAsync(new Guid(releaseId), Include.Artists | Include.ReleaseGroups, cancellationToken).ConfigureAwait(false);
+            var releaseResult = await query.LookupReleaseAsync(new Guid(releaseId), Include.Artists | Include.ReleaseGroups, cancellationToken).ConfigureAwait(false);
             return GetReleaseResult(releaseResult).SingleItemAsEnumerable();
         }
 
         if (!string.IsNullOrEmpty(releaseGroupId))
         {
-            var releaseGroupResult = await _musicBrainzQuery.LookupReleaseGroupAsync(new Guid(releaseGroupId), Include.Releases, null, cancellationToken).ConfigureAwait(false);
+            var releaseGroupResult = await query.LookupReleaseGroupAsync(new Guid(releaseGroupId), Include.Releases, null, cancellationToken).ConfigureAwait(false);
 
             // No need to pass the cancellation token to GetReleaseGroupResultAsync as we're already passing it to ToBlockingEnumerable
             return GetReleaseGroupResultAsync(releaseGroupResult.Releases, CancellationToken.None).ToBlockingEnumerable(cancellationToken);
@@ -93,7 +53,7 @@ public class MusicBrainzAlbumProvider : IRemoteMetadataProvider<MusicAlbum, Albu
 
         if (!string.IsNullOrWhiteSpace(artistMusicBrainzId))
         {
-            var releaseSearchResults = await _musicBrainzQuery.FindReleasesAsync($"\"{searchInfo.Name}\" AND arid:{artistMusicBrainzId}", null, null, false, cancellationToken)
+            var releaseSearchResults = await query.FindReleasesAsync($"\"{searchInfo.Name}\" AND arid:{artistMusicBrainzId}", null, null, false, cancellationToken)
                 .ConfigureAwait(false);
 
             if (releaseSearchResults.Results.Count > 0)
@@ -106,7 +66,7 @@ public class MusicBrainzAlbumProvider : IRemoteMetadataProvider<MusicAlbum, Albu
             // I'm sure there is a better way but for now it resolves search for 12" Mixes
             var queryName = searchInfo.Name.Replace("\"", string.Empty, StringComparison.Ordinal);
 
-            var releaseSearchResults = await _musicBrainzQuery.FindReleasesAsync($"\"{queryName}\" AND artist:\"{searchInfo.GetAlbumArtist()}\"c", null, null, false, cancellationToken)
+            var releaseSearchResults = await query.FindReleasesAsync($"\"{queryName}\" AND artist:\"{searchInfo.GetAlbumArtist()}\"c", null, null, false, cancellationToken)
                 .ConfigureAwait(false);
 
             if (releaseSearchResults.Results.Count > 0)
@@ -138,10 +98,11 @@ public class MusicBrainzAlbumProvider : IRemoteMetadataProvider<MusicAlbum, Albu
             yield break;
         }
 
+        var query = MusicBrainz.Plugin.Instance!.MusicBrainzQuery;
         foreach (var result in releaseSearchResults)
         {
             // Fetch full release info, otherwise artists are missing
-            var fullResult = await _musicBrainzQuery.LookupReleaseAsync(result.Id, Include.Artists | Include.ReleaseGroups, cancellationToken).ConfigureAwait(false);
+            var fullResult = await query.LookupReleaseAsync(result.Id, Include.Artists | Include.ReleaseGroups, cancellationToken).ConfigureAwait(false);
             yield return GetReleaseResult(fullResult);
         }
     }
@@ -195,6 +156,7 @@ public class MusicBrainzAlbumProvider : IRemoteMetadataProvider<MusicAlbum, Albu
     public async Task<MetadataResult<MusicAlbum>> GetMetadata(AlbumInfo info, CancellationToken cancellationToken)
     {
         // TODO: This sets essentially nothing. As-is, it's mostly useless. Make it actually pull metadata and use it.
+        var query = MusicBrainz.Plugin.Instance!.MusicBrainzQuery;
         var releaseId = info.GetReleaseId();
         var releaseGroupId = info.GetReleaseGroupId();
 
@@ -207,7 +169,7 @@ public class MusicBrainzAlbumProvider : IRemoteMetadataProvider<MusicAlbum, Albu
         if (string.IsNullOrWhiteSpace(releaseId) && !string.IsNullOrWhiteSpace(releaseGroupId))
         {
             // TODO: Actually try to match the release. Simply taking the first result is stupid.
-            var releaseGroup = await _musicBrainzQuery.LookupReleaseGroupAsync(new Guid(releaseGroupId), Include.None, null, cancellationToken).ConfigureAwait(false);
+            var releaseGroup = await query.LookupReleaseGroupAsync(new Guid(releaseGroupId), Include.None, null, cancellationToken).ConfigureAwait(false);
             var release = releaseGroup.Releases?.Count > 0 ? releaseGroup.Releases[0] : null;
             if (release is not null)
             {
@@ -224,13 +186,13 @@ public class MusicBrainzAlbumProvider : IRemoteMetadataProvider<MusicAlbum, Albu
 
             if (!string.IsNullOrEmpty(artistMusicBrainzId))
             {
-                var releaseSearchResults = await _musicBrainzQuery.FindReleasesAsync($"\"{info.Name}\" AND arid:{artistMusicBrainzId}", null, null, false, cancellationToken)
+                var releaseSearchResults = await query.FindReleasesAsync($"\"{info.Name}\" AND arid:{artistMusicBrainzId}", null, null, false, cancellationToken)
                     .ConfigureAwait(false);
                 releaseResult = releaseSearchResults.Results.Count > 0 ? releaseSearchResults.Results[0].Item : null;
             }
             else if (!string.IsNullOrEmpty(info.GetAlbumArtist()))
             {
-                var releaseSearchResults = await _musicBrainzQuery.FindReleasesAsync($"\"{info.Name}\" AND artist:{info.GetAlbumArtist()}", null, null, false, cancellationToken)
+                var releaseSearchResults = await query.FindReleasesAsync($"\"{info.Name}\" AND artist:{info.GetAlbumArtist()}", null, null, false, cancellationToken)
                     .ConfigureAwait(false);
                 releaseResult = releaseSearchResults.Results.Count > 0 ? releaseSearchResults.Results[0].Item : null;
             }
@@ -253,7 +215,7 @@ public class MusicBrainzAlbumProvider : IRemoteMetadataProvider<MusicAlbum, Albu
         // If we have a release ID but not a release group ID, lookup the release group
         if (!string.IsNullOrWhiteSpace(releaseId) && string.IsNullOrWhiteSpace(releaseGroupId))
         {
-            var release = await _musicBrainzQuery.LookupReleaseAsync(new Guid(releaseId), Include.ReleaseGroups, cancellationToken).ConfigureAwait(false);
+            var release = await query.LookupReleaseAsync(new Guid(releaseId), Include.ReleaseGroups, cancellationToken).ConfigureAwait(false);
             releaseGroupId = release.ReleaseGroup?.Id.ToString();
             result.HasMetadata = true;
         }
@@ -285,23 +247,4 @@ public class MusicBrainzAlbumProvider : IRemoteMetadataProvider<MusicAlbum, Albu
     {
         throw new NotImplementedException();
     }
-
-    /// <inheritdoc />
-    public void Dispose()
-    {
-        Dispose(true);
-        GC.SuppressFinalize(this);
-    }
-
-    /// <summary>
-    /// Dispose all resources.
-    /// </summary>
-    /// <param name="disposing">Whether to dispose.</param>
-    protected virtual void Dispose(bool disposing)
-    {
-        if (disposing)
-        {
-            _musicBrainzQuery.Dispose();
-        }
-    }
 }
diff --git a/MediaBrowser.Providers/Plugins/MusicBrainz/MusicBrainzArtistProvider.cs b/MediaBrowser.Providers/Plugins/MusicBrainz/MusicBrainzArtistProvider.cs
index 9df21596c5..0fe4e6bb16 100644
--- a/MediaBrowser.Providers/Plugins/MusicBrainz/MusicBrainzArtistProvider.cs
+++ b/MediaBrowser.Providers/Plugins/MusicBrainz/MusicBrainzArtistProvider.cs
@@ -8,37 +8,19 @@ using Jellyfin.Extensions;
 using MediaBrowser.Controller.Entities.Audio;
 using MediaBrowser.Controller.Providers;
 using MediaBrowser.Model.Entities;
-using MediaBrowser.Model.Plugins;
 using MediaBrowser.Model.Providers;
 using MediaBrowser.Providers.Music;
-using MediaBrowser.Providers.Plugins.MusicBrainz.Configuration;
 using MetaBrainz.MusicBrainz;
 using MetaBrainz.MusicBrainz.Interfaces.Entities;
 using MetaBrainz.MusicBrainz.Interfaces.Searches;
-using Microsoft.Extensions.Logging;
 
 namespace MediaBrowser.Providers.Plugins.MusicBrainz;
 
 /// <summary>
 /// MusicBrainz artist provider.
 /// </summary>
-public class MusicBrainzArtistProvider : IRemoteMetadataProvider<MusicArtist, ArtistInfo>, IDisposable, IHasOrder
+public class MusicBrainzArtistProvider : IRemoteMetadataProvider<MusicArtist, ArtistInfo>, IHasOrder
 {
-    private readonly ILogger<MusicBrainzArtistProvider> _logger;
-    private Query _musicBrainzQuery;
-
-    /// <summary>
-    /// Initializes a new instance of the <see cref="MusicBrainzArtistProvider"/> class.
-    /// </summary>
-    /// <param name="logger">The logger.</param>
-    public MusicBrainzArtistProvider(ILogger<MusicBrainzArtistProvider> logger)
-    {
-        _logger = logger;
-        _musicBrainzQuery = new Query();
-        ReloadConfig(null, MusicBrainz.Plugin.Instance!.Configuration);
-        MusicBrainz.Plugin.Instance!.ConfigurationChanged += ReloadConfig;
-    }
-
     /// <inheritdoc />
     public string Name => "MusicBrainz";
 
@@ -46,41 +28,19 @@ public class MusicBrainzArtistProvider : IRemoteMetadataProvider<MusicArtist, Ar
     /// Runs first to populate the MusicBrainz artist ID used by downstream providers.
     public int Order => 0;
 
-    private void ReloadConfig(object? sender, BasePluginConfiguration e)
-    {
-        var configuration = (PluginConfiguration)e;
-        if (Uri.TryCreate(configuration.Server, UriKind.Absolute, out var server))
-        {
-            Query.DefaultServer = server.DnsSafeHost;
-            Query.DefaultPort = server.Port;
-            Query.DefaultUrlScheme = server.Scheme;
-        }
-        else
-        {
-            // Fallback to official server
-            _logger.LogWarning("Invalid MusicBrainz server specified, falling back to official server");
-            var defaultServer = new Uri(PluginConfiguration.DefaultServer);
-            Query.DefaultServer = defaultServer.Host;
-            Query.DefaultPort = defaultServer.Port;
-            Query.DefaultUrlScheme = defaultServer.Scheme;
-        }
-
-        Query.DelayBetweenRequests = configuration.RateLimit;
-        _musicBrainzQuery = new Query();
-    }
-
     /// <inheritdoc />
     public async Task<IEnumerable<RemoteSearchResult>> GetSearchResults(ArtistInfo searchInfo, CancellationToken cancellationToken)
     {
+        var query = MusicBrainz.Plugin.Instance!.MusicBrainzQuery;
         var artistId = searchInfo.GetMusicBrainzArtistId();
 
         if (!string.IsNullOrWhiteSpace(artistId))
         {
-            var artistResult = await _musicBrainzQuery.LookupArtistAsync(new Guid(artistId), Include.Aliases, null, null, cancellationToken).ConfigureAwait(false);
+            var artistResult = await query.LookupArtistAsync(new Guid(artistId), Include.Aliases, null, null, cancellationToken).ConfigureAwait(false);
             return GetResultFromResponse(artistResult).SingleItemAsEnumerable();
         }
 
-        var artistSearchResults = await _musicBrainzQuery.FindArtistsAsync($"\"{searchInfo.Name}\"", null, null, false, cancellationToken)
+        var artistSearchResults = await query.FindArtistsAsync($"\"{searchInfo.Name}\"", null, null, false, cancellationToken)
             .ConfigureAwait(false);
         if (artistSearchResults.Results.Count > 0)
         {
@@ -90,7 +50,7 @@ public class MusicBrainzArtistProvider : IRemoteMetadataProvider<MusicArtist, Ar
         if (searchInfo.Name.HasDiacritics())
         {
             // Try again using the search with an accented characters query
-            var artistAccentsSearchResults = await _musicBrainzQuery.FindArtistsAsync($"artistaccent:\"{searchInfo.Name}\"", null, null, false, cancellationToken)
+            var artistAccentsSearchResults = await query.FindArtistsAsync($"artistaccent:\"{searchInfo.Name}\"", null, null, false, cancellationToken)
                 .ConfigureAwait(false);
             if (artistAccentsSearchResults.Results.Count > 0)
             {
@@ -168,23 +128,4 @@ public class MusicBrainzArtistProvider : IRemoteMetadataProvider<MusicArtist, Ar
     {
         throw new NotImplementedException();
     }
-
-    /// <inheritdoc />
-    public void Dispose()
-    {
-        Dispose(true);
-        GC.SuppressFinalize(this);
-    }
-
-    /// <summary>
-    /// Dispose all resources.
-    /// </summary>
-    /// <param name="disposing">Whether to dispose.</param>
-    protected virtual void Dispose(bool disposing)
-    {
-        if (disposing)
-        {
-            _musicBrainzQuery.Dispose();
-        }
-    }
 }
diff --git a/MediaBrowser.Providers/Plugins/MusicBrainz/Plugin.cs b/MediaBrowser.Providers/Plugins/MusicBrainz/Plugin.cs
index 39cfd727f3..69225d0b95 100644
--- a/MediaBrowser.Providers/Plugins/MusicBrainz/Plugin.cs
+++ b/MediaBrowser.Providers/Plugins/MusicBrainz/Plugin.cs
@@ -1,6 +1,8 @@
 using System;
 using System.Collections.Generic;
+using System.Diagnostics.CodeAnalysis;
 using System.Net.Http.Headers;
+using System.Threading;
 using MediaBrowser.Common;
 using MediaBrowser.Common.Configuration;
 using MediaBrowser.Common.Plugins;
@@ -8,30 +10,42 @@ using MediaBrowser.Model.Plugins;
 using MediaBrowser.Model.Serialization;
 using MediaBrowser.Providers.Plugins.MusicBrainz.Configuration;
 using MetaBrainz.MusicBrainz;
+using Microsoft.Extensions.Logging;
 
 namespace MediaBrowser.Providers.Plugins.MusicBrainz;
 
 /// <summary>
 /// Plugin instance.
 /// </summary>
-public class Plugin : BasePlugin<PluginConfiguration>, IHasWebPages
+public class Plugin : BasePlugin<PluginConfiguration>, IHasWebPages, IDisposable
 {
+    private readonly ILogger<Plugin> _logger;
+    private readonly Lock _queryLock = new();
+    private Query _musicBrainzQuery;
+    private bool _disposed;
+
     /// <summary>
     /// Initializes a new instance of the <see cref="Plugin"/> class.
     /// </summary>
     /// <param name="applicationPaths">Instance of the <see cref="IApplicationPaths"/> interface.</param>
     /// <param name="xmlSerializer">Instance of the <see cref="IXmlSerializer"/> interface.</param>
     /// <param name="applicationHost">Instance of the <see cref="IApplicationHost"/> interface.</param>
-    public Plugin(IApplicationPaths applicationPaths, IXmlSerializer xmlSerializer, IApplicationHost applicationHost)
+    /// <param name="logger">Instance of the <see cref="ILogger{Plugin}"/> interface.</param>
+    public Plugin(IApplicationPaths applicationPaths, IXmlSerializer xmlSerializer, IApplicationHost applicationHost, ILogger<Plugin> logger)
         : base(applicationPaths, xmlSerializer)
     {
         Instance = this;
+        _logger = logger;
 
         // TODO: Change this to "JellyfinMusicBrainzPlugin" once we take it out of the server repo.
         Query.DefaultUserAgent.Add(new ProductInfoHeaderValue(applicationHost.Name.Replace(' ', '-'), applicationHost.ApplicationVersionString));
         Query.DefaultUserAgent.Add(new ProductInfoHeaderValue($"({applicationHost.ApplicationUserAgentAddress})"));
-        Query.DelayBetweenRequests = Instance.Configuration.RateLimit;
-        Query.DefaultServer = Instance.Configuration.Server;
+
+        ApplyServerConfig(Configuration);
+        Query.DelayBetweenRequests = Configuration.RateLimit;
+        _musicBrainzQuery = new Query();
+
+        ConfigurationChanged += OnConfigurationChanged;
     }
 
     /// <summary>
@@ -52,6 +66,25 @@ public class Plugin : BasePlugin<PluginConfiguration>, IHasWebPages
     // TODO remove when plugin removed from server.
     public override string ConfigurationFileName => "Jellyfin.Plugin.MusicBrainz.xml";
 
+    /// <summary>
+    /// Gets the current MusicBrainz query client.
+    /// </summary>
+    /// <remarks>
+    /// Always read this property anew before each request — the underlying instance is
+    /// replaced when the server URL changes. Old instances are intentionally left alive
+    /// so in-flight requests can finish; their unmanaged resources leak until GC.
+    /// </remarks>
+    public Query MusicBrainzQuery
+    {
+        get
+        {
+            lock (_queryLock)
+            {
+                return _musicBrainzQuery;
+            }
+        }
+    }
+
     /// <inheritdoc />
     public IEnumerable<PluginPageInfo> GetPages()
     {
@@ -61,4 +94,65 @@ public class Plugin : BasePlugin<PluginConfiguration>, IHasWebPages
             EmbeddedResourcePath = GetType().Namespace + ".Configuration.config.html"
         };
     }
+
+    /// <inheritdoc />
+    public void Dispose()
+    {
+        Dispose(true);
+        GC.SuppressFinalize(this);
+    }
+
+    /// <summary>
+    /// Releases unmanaged and managed resources.
+    /// </summary>
+    /// <param name="disposing">Whether to dispose managed resources.</param>
+    protected virtual void Dispose(bool disposing)
+    {
+        if (_disposed)
+        {
+            return;
+        }
+
+        if (disposing)
+        {
+            ConfigurationChanged -= OnConfigurationChanged;
+            lock (_queryLock)
+            {
+                _musicBrainzQuery.Dispose();
+            }
+        }
+
+        _disposed = true;
+    }
+
+    [SuppressMessage("IDisposableAnalyzers.Correctness", "IDISP003:Dispose previous before re-assigning", Justification = "The previous Query may still be in use by in-flight async requests; disposing it would cause ObjectDisposedException. The orphan is intentionally left for GC.")]
+    private void OnConfigurationChanged(object? sender, BasePluginConfiguration e)
+    {
+        var configuration = (PluginConfiguration)e;
+        ApplyServerConfig(configuration);
+        Query.DelayBetweenRequests = configuration.RateLimit;
+
+        lock (_queryLock)
+        {
+            _musicBrainzQuery = new Query();
+        }
+    }
+
+    private void ApplyServerConfig(PluginConfiguration configuration)
+    {
+        if (Uri.TryCreate(configuration.Server, UriKind.Absolute, out var server))
+        {
+            Query.DefaultServer = server.DnsSafeHost;
+            Query.DefaultPort = server.Port;
+            Query.DefaultUrlScheme = server.Scheme;
+        }
+        else
+        {
+            _logger.LogWarning("Invalid MusicBrainz server specified, falling back to official server");
+            var defaultServer = new Uri(PluginConfiguration.DefaultServer);
+            Query.DefaultServer = defaultServer.Host;
+            Query.DefaultPort = defaultServer.Port;
+            Query.DefaultUrlScheme = defaultServer.Scheme;
+        }
+    }
 }