-
Notifications
You must be signed in to change notification settings - Fork 105
Async disposal on .NET 6 or later #262
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,72 @@ | ||
using Microsoft.Extensions.DependencyInjection; | ||
using Microsoft.Extensions.Logging; | ||
using Serilog.Core; | ||
using Serilog.Events; | ||
using Xunit; | ||
|
||
namespace Serilog.Extensions.Logging.Tests; | ||
|
||
public class DisposeTests | ||
{ | ||
private readonly DisposableSink _sink; | ||
private readonly Logger _serilogLogger; | ||
|
||
public DisposeTests() | ||
{ | ||
_sink = new DisposableSink(); | ||
_serilogLogger = new LoggerConfiguration() | ||
.WriteTo.Sink(_sink) | ||
.CreateLogger(); | ||
} | ||
|
||
[Fact] | ||
public void AddSerilog_must_dispose_the_provider_when_dispose_is_true() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The Serilog codebases use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry, I will address! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Naming changed |
||
{ | ||
var services = new ServiceCollection() | ||
.AddLogging(builder => builder.AddSerilog(logger: _serilogLogger, dispose: true)) | ||
.BuildServiceProvider(); | ||
|
||
// Get a logger so that we ensure SerilogLoggerProvider is created | ||
var logger = services.GetRequiredService<ILogger<DisposeTests>>(); | ||
logger.LogInformation("Hello, world!"); | ||
|
||
services.Dispose(); | ||
Assert.True(_sink.DisposeCalled); | ||
Assert.False(_sink.DisposeAsyncCalled); | ||
} | ||
|
||
#if NET8_0_OR_GREATER | ||
[Fact] | ||
public async Task AddSerilog_must_async_dispose_the_provider_when_dispose_is_true() | ||
{ | ||
var services = new ServiceCollection() | ||
.AddLogging(builder => builder.AddSerilog(logger: _serilogLogger, dispose: true)) | ||
.BuildServiceProvider(); | ||
|
||
// Get a logger so that we ensure SerilogLoggerProvider is created | ||
var logger = services.GetRequiredService<ILogger<DisposeTests>>(); | ||
logger.LogInformation("Hello, world!"); | ||
|
||
await services.DisposeAsync(); | ||
Assert.False(_sink.DisposeCalled); | ||
Assert.True(_sink.DisposeAsyncCalled); | ||
} | ||
#endif | ||
|
||
private sealed class DisposableSink : ILogEventSink, IDisposable, IAsyncDisposable | ||
{ | ||
public bool DisposeAsyncCalled { get; private set; } | ||
public bool DisposeCalled { get; private set; } | ||
|
||
public void Dispose() => DisposeCalled = true; | ||
public ValueTask DisposeAsync() | ||
{ | ||
DisposeAsyncCalled = true; | ||
return default; | ||
} | ||
|
||
public void Emit(LogEvent logEvent) | ||
{ | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,56 @@ | ||
using Microsoft.Extensions.DependencyInjection; | ||
using Microsoft.Extensions.Logging; | ||
using Serilog.Extensions.Logging.Tests.Support; | ||
using Xunit; | ||
|
||
namespace Serilog.Extensions.Logging.Tests; | ||
|
||
public class SerilogLoggingBuilderExtensionsTests | ||
{ | ||
[Fact] | ||
public void AddSerilog_must_register_a_ILoggerProvider() | ||
{ | ||
var services = new ServiceCollection() | ||
.AddLogging(builder => { builder.AddSerilog(); }) | ||
.BuildServiceProvider(); | ||
|
||
var loggerProviders = services.GetServices<ILoggerProvider>(); | ||
Assert.Contains(loggerProviders, provider => provider is SerilogLoggerProvider); | ||
} | ||
|
||
[Fact] | ||
public void AddSerilog_must_register_a_ILoggerProvider_that_forwards_logs_to_static_Serilog_Logger() | ||
{ | ||
var sink = new SerilogSink(); | ||
Log.Logger = new LoggerConfiguration() | ||
.WriteTo.Sink(sink) | ||
.CreateLogger(); | ||
|
||
var services = new ServiceCollection() | ||
.AddLogging(builder => { builder.AddSerilog(); }) | ||
.BuildServiceProvider(); | ||
|
||
var logger = services.GetRequiredService<ILogger<SerilogLoggingBuilderExtensionsTests>>(); | ||
logger.LogInformation("Hello, world!"); | ||
|
||
Assert.Single(sink.Writes); | ||
} | ||
|
||
[Fact] | ||
public void AddSerilog_must_register_a_ILoggerProvider_that_forwards_logs_to_provided_logger() | ||
{ | ||
var sink = new SerilogSink(); | ||
var serilogLogger = new LoggerConfiguration() | ||
.WriteTo.Sink(sink) | ||
.CreateLogger(); | ||
|
||
var services = new ServiceCollection() | ||
.AddLogging(builder => { builder.AddSerilog(logger: serilogLogger); }) | ||
.BuildServiceProvider(); | ||
|
||
var logger = services.GetRequiredService<ILogger<SerilogLoggingBuilderExtensionsTests>>(); | ||
logger.LogInformation("Hello, world!"); | ||
|
||
Assert.Single(sink.Writes); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Serilog projects define constants for each conditional feature centrally in the CSPROJ, e.g.:
https://github.com/serilog/serilog/blob/dev/src/Serilog/Serilog.csproj
Could we define
FEATURE_ASYNCDISPOSABLE
and use it in place of the version constraints in the code?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Uh, dooh. Sorry about that. I had noticed that the Serilog core code base used such flags, but forgot to check this code. I will address.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have defined FEATURE_ASYNCDISPOSABLE now