Async disposal on .NET 6 or later #262
Conversation
| @@ -14,13 +15,19 @@ namespace Serilog.Extensions.Logging; | |||
| /// </summary> | |||
| [ProviderAlias("Serilog")] | |||
| public class SerilogLoggerProvider : ILoggerProvider, ILogEventEnricher, ISupportExternalScope | |||
| #if NET6_0_OR_GREATER | |||
There was a problem hiding this comment.
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.
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.
I have defined FEATURE_ASYNCDISPOSABLE now
| } | ||
|
|
||
| [Fact] | ||
| public void AddSerilog_must_dispose_the_provider_when_dispose_is_true() |
There was a problem hiding this comment.
The Serilog codebases use PascalCaseForTestNames(); shortening this to something like DisposesProviderWhenDisposeIsTrue() would help make the scheme work.
There was a problem hiding this comment.
Sorry, I will address!
Currently
SerilogLoggerProvideronly implementsIDisposablewhich means that sinks that implementIAsyncDisposablewill not be disposed async whendispose=trueis passed toAddSerilog().In this PR I have opt'ed to only implement
IAsyncDisposablefor .NET 6 or later as we can't use the staticLog.CloseAndFlushAsync()method on .NET Framework. When an existingILoggeris provided toAddSerilog()anddispose=truewe will now try to callIAsyncDisposable.DisposeAsync()and fallback toIDisposable.Dispose().If no existing
ILoggeris provided we will useLog.CloseAndFlushAsync()for async disposal.The PR also includes tests of the disposal functionality, as well as some more high level "functional" tests of the functionality implemented in
AddSerilog(), mainly forwarding of logs fromMS..Extensions.ILogger<T>to Serilog sink.This change is related to a similar change I made in App Insights sink, see serilog-contrib/serilog-sinks-applicationinsights#228.