`SerilogLoggerProvider` should dispose the associated logger, if set

[nblumhardt]

Via: http://stackoverflow.com/questions/38286140/how-to-dispose-of-serilog-2-0-correctly-in-asp-net-core-application

If the provider is using the static Log pipeline, the application should be responsible for disposal as there's no indication of any ownership transfer.

If a disposable Logger instance is passed in, disposing it is reasonable. To make this explicit, we could:

1. overload AddSerilog() with a method specifically accepting Logger
2. or, make the overload generic where TLogger: ILogger, IDisposable
3. or, add a dispose: default Boolean parameter to AddSerilog()
4. or, add a new method like AddSerilogAndDispose() for the purpose.

The decision here depends on whether we want to move towards disposing by default (1 or 2) or not (3 or 4).

If we want to dispose by default, (1) is probably the friendliest option, while (3) seems like the tidiest opt-in mechanism.

[colin-young]

Aren't 1 & 2 also opt-in? Unless I'm missing something. The idea is to provide AddSerilog() and AddSerilog(ILogger logger) or AddSerilog<T>(T logger) where T: ILogger, IDisposable, correct? If so, calling the overloaded method is opting-in. I'm not a fan of option AddSerilogAndDispose().

I'm not in the logging head-space right now (or any code head-space -- thank you very much NuGet/TeamCity/Visual Studio), but would any of those options have any negative impacts on dependency injection configured code? I'm thinking no since you'd simply not opt-in and let your container manage disposal, but I've been having some issues with Autofac and getting CloseAndFlush() working together nicely, so maybe one would opt-in to letting Serilog dispose for you (of course that's probably more my fault than either Serilog or Autofac, I just haven't had a chance to really dig into it yet).

[nblumhardt]

Thanks for checking it out @colin-young. The first two would implicitly opt you in, because when the return value of CreateLogger() is passed through (same API as today) overload resolution would pick the more specific overload, and this would be the disposing one.

(Are you using https://github.com/nblumhardt/autofac-serilog-integration/, BTW?)

[colin-young]

I'm a bit confused now. I just actually took a look at the code, and there is already a public static ILoggerFactory AddSerilog(this ILoggerFactory factory, ILogger logger = null) in there. Are we talking about modifying that method to track that we passed in an IDisposable logger, and modifying the Dispose() method? Or am I being thick and missing something obvious here?

In any case, I'm starting to lean more to #3 (dispose: default bool param) as explicit opt-in rather than an implicit opt-in. I think the implicit option could cause some unexpected side-effects in certain situations.

I'm using a modified version of https://github.com/nblumhardt/autofac-serilog-integration/. The stock one didn't quite fit in with what I was trying to accomplish (I'll allow that it could have been a PEBKAC), so I'm really just using the AttachToComponentRegistration implementation (with proper attribution of course). It does look like there are a couple recent commits I need to incorporate though.

[nblumhardt]

The difference is ILogger logger vs Logger logger: Serilog 2 returns the concrete, disposable type from CreateLogger(), but the abstract, non-disposable type from ForContext() and friends. Based on your first impressions I think it's probably too subtle, and dispose: true is a better API 👍

(Drop me a line if I can help with the Autofac/Serilog bit. Haven't revisited the adapter in a while, could probably use an update 😄 )

[nblumhardt]

Since this is now opt-in, heading for enhancement rather than bug.

What should dispose: true do if the logger is not passed explicitly (i.e. should it call Log.CloseAndFlush())? Probably a 'yes'..?

[colin-young]

Yeah, I didn't look at the return types, just the inputs :) That is pretty subtle and just makes the case for unintended consequences stronger.

I think the correct action on dispose: true is to Log.CloseAndFlush() since there's going to be documentation on the dispose parameter explaining that that is what it's going to do, right?

I think we'd want to default to false if a logger is not provided, true otherwise:

public static ILoggerFactory AddSerilog(
            this ILoggerFactory factory,
            ILogger logger,
            bool dispose = false)
{...}

public static ILoggerFactory AddSerilog(
            this ILoggerFactory factory,
            bool dispose = true)
{
    AddSerilog(factory, null, dispose);
}

[colin-young]

As for my Autofac/Serilog stuff, I need to revisit that code. I started it way, way back when Core support in Serilog first appeared and I've got configuration/options stuff to clean up there. Basically, I wrote a module that by default sets up Serilog to log to a sensible location on disk, adds an encrypting destructuring policy (we're in healthcare), an event type enricher and log context enricher and will override with settings from a Json configuration file (defaulting to logging,json, but overridable in the module registration), all by simply registering the logging Autofac module. My requirement was to have a one-liner to add logging to any of our projects and have it "just work" by default. It works very well, but some bits are a bit convoluted, and we've got some issues with the options model, especially as it pertains to our port of the SQL Server sink, which we did before the official one got ported (and, at the time, I wasn't sure that ours wouldn't have special requirements that were not generally useful to the general sink).

[nblumhardt]

Thanks for the notes. I think we need to use false as a default across-the-board because the subtle breaking change could be a problem for existing code. I guess the more I look at it, the less I can see AddSerilog() giving any clue as to an ownership transfer occurring, without the explicit flag being present at the call.

I think I'll merge this through to dev so we can kick the tyres and get some experience with it out in the wild :-)