angelobreuer / Lavalink4NET

Lavalink4NET is a Lavalink wrapper with node clustering, caching and custom players for .NET with support for Discord.Net, DSharpPlus, Remora, and NetCord.
https://lavalink4net.angelobreuer.de/
MIT License
153 stars 27 forks source link

Inactivity Tracking Service requires Lavalink4NET logger (though it should be optional) #6

Closed Mijago closed 5 years ago

Mijago commented 5 years ago

Describe the bug Inactivity Tracking Service requires Lavalink4NET logger (though it should be optional). I registered the following code:

                .AddSingleton<InactivityTrackingOptions>()
                .AddSingleton<InactivityTrackingService>()

And later used:

            _provider.GetRequiredService<InactivityTrackingService>().BeginTracking();

Then I get this nice error message:

Unhandled Exception: System.InvalidOperationException: Unable to resolve service for type 'Lavalink4NET.ILogger' while attempting to activate 'Lavalink4NET.Tracking.InactivityTrackingService'.
   at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteFactory.CreateArgumentCallSites(Type serviceType, Type implementationType, CallSiteChain callSiteChain, ParameterInfo[] parameters, Boolean throwIfCallSiteNotFound)
   at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteFactory.CreateConstructorCallSite(Type serviceType, Type implementationType, CallSiteChain callSiteChain)
   at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteFactory.TryCreateExact(ServiceDescriptor descriptor, Type serviceType, CallSiteChain callSiteChain)
   at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteFactory.TryCreateExact(Type serviceType, CallSiteChain callSiteChain)
   at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteFactory.CreateCallSite(Type serviceType, CallSiteChain callSiteChain)
   at Microsoft.Extensions.DependencyInjection.ServiceLookup.ServiceProviderEngine.CreateServiceAccessor(Type serviceType)
   at System.Collections.Concurrent.ConcurrentDictionary`2.GetOrAdd(TKey key, Func`2 valueFactory)
   at Microsoft.Extensions.DependencyInjection.ServiceLookup.ServiceProviderEngine.GetService(Type serviceType, ServiceProviderEngineScope serviceProviderEngineScope)
   at Microsoft.Extensions.DependencyInjection.ServiceProviderServiceExtensions.GetRequiredService(IServiceProvider provider, Type serviceType)
   at Microsoft.Extensions.DependencyInjection.ServiceProviderServiceExtensions.GetRequiredService[T](IServiceProvider provider)
   at MusicBot.Services.StartupService.InitializeLavalink() in /app/Services/StartupService.cs:line 69
   at MusicBot.Services.StartupService.StartAsync() in /app/Services/StartupService.cs:line 64
   at MusicBot.Startup.RunAsync() in /app/Startup.cs:line 47
   at MusicBot.Startup.RunAsync(String[] args) in /app/Startup.cs:line 31
   at MusicBot.Program.<Main>(String[] args)

To Reproduce

Expected behavior It should run without requiring a logger.

(please complete the following information):

Additional context I'll just solve the issue on my side by adding a Lavalink4NET.ILogger, but the wiki states that logging is optional, so I just wanted to let you know that here may exist an issue.

Mijago commented 5 years ago

This gives me another question: I add my ILogger to my provider by using .AddLogging(builder => builder.AddConsole()) (for testing etc.).

Then I register Lavalink-related stuff using:

.AddSingleton<IAudioService, LavalinkNode>()
.AddSingleton<IDiscordClientWrapper, DiscordClientWrapper>()
.AddSingleton(new LavalinkNodeOptions
{
    RestUri = "http://lavalink:8080/",
    WebSocketUri = "ws://lavalink:8080/",
    Password = "youshallnotpass",
    AllowResuming = false // (btw, I can pause/resume despite this setting)
})
.AddSingleton<InactivityTrackingOptions>() // This is new
.AddSingleton<InactivityTrackingService>() // This is new, too

So my question is: How do I get the logger to the LavalinkNode instance when using DI?

Offtopic: By the way, good work on this library! I enjoy working with it.

angelobreuer commented 5 years ago

The problem is with Dependency Injection, for example when creating the instance of InactivityTrackingService using its constructor the issue is solved, I added to the 1.3.0-release milestone adding service provider constructors (or constructors without the optional parameters) for use with dependency injection.

As a "work-around" you could use the constructor of the InactivityTrackingService to avoid the ILogger as shown below:

new ServiceCollection()
    // [...] adding client wrapper, etc.
    .AddSingleton(sp => new InactivityTrackingService(sp.GetRequiredService<IAudioService>(),
        sp.GetRequiredService<IDiscordClientWrapper>(),
        sp.GetRequiredService<InactivityTrackingOptions>(),
        null))
    // [...]
    .BuildServiceProvider();
angelobreuer commented 5 years ago

This gives me another question: I add my ILogger to my provider by using .AddLogging(builder => builder.AddConsole()) (for testing etc.).

Then I register Lavalink-related stuff using:

.AddSingleton<IAudioService, LavalinkNode>()
.AddSingleton<IDiscordClientWrapper, DiscordClientWrapper>()
.AddSingleton(new LavalinkNodeOptions
{
    RestUri = "http://lavalink:8080/",
    WebSocketUri = "ws://lavalink:8080/",
    Password = "youshallnotpass",
    AllowResuming = false // (btw, I can pause/resume despite this setting)
})
.AddSingleton<InactivityTrackingOptions>() // This is new
.AddSingleton<InactivityTrackingService>() // This is new, too

So my question is: How do I get the logger to the LavalinkNode instance when using DI?

Offtopic: By the way, good work on this library! I enjoy working with it.

The ILogger-interface used by Lavalink4NET is not from Microsoft.Extensions.Logging(.Abstractions) anymore (since version 1.2.0, to remove the dependency).

See: https://github.com/angelobreuer/Lavalink4NET/blob/master/samples/Lavalink4NET.Discord_NET.ExampleBot/CustomLogger.cs for an example implementation of the interface. (It must be also added to the dependency injection service provider)


By the way:

AllowResuming = false // (btw, I can pause/resume despite this setting)

A note on this setting, this setting enables the session resume after reconnecting to the lavalink node to keep the player data (enqueued songs, etc.). Disabling the option will disconnect all players when stopping the bot.

angelobreuer commented 5 years ago

Hope the issue is fixed now with #8, release version 1.3.0 is now available (also on NuGet).