Stack Overflow when using a newer version of Serilog.Sinks.PeriodicBatching

[martin-pronestor]

When using Serilog.Sinks.Slack together with the latest version of Serilog.Sinks.PeriodicBatching the call to Serilog.Log.CloseAndFlush() will crash with a StackOverflowException because the SlackSink Dispose method will call Dispose on PeriodicBatchSink which in turn calls Dispose on SlackSink, causing an endless loop.

This issue was discovered while using the Sink in a .NET Framework application where transient dependencies have to be explicitly included and where the dependency was updated to the latest version.

From my research into how PeriodicBatching code works and how other Sinks handle this it seems that maybe SinkSlack should be split up so that it doesn't pass a reference to itself to PeriodicBatchingSink. But I'm not sure how this change would affect people who use this library today.

Here is the sample code I used to reproduce the issue:

using Serilog;
using Serilog.Events;
using Serilog.Sinks.Slack;
using Serilog.Sinks.Slack.Models;

namespace SlackSinkStackOverflow;

internal static class Program
{
    private static void Main()
    {
        Log.Logger = new LoggerConfiguration()
            .WriteTo.Console()
            .WriteTo.Slack(new SlackSinkOptions
                { WebHookUrl = "https://hooks.slack.com/services/...", MinimumLogEventLevel = LogEventLevel.Error })
            .CreateLogger();

        try
        {
            Log.Information("Starting application");
        }
        catch (Exception exception)
        {
            Log.Fatal(exception, "Application terminated unexpectedly");
        }
        finally
        {
            // Causes a StackOverflowException
            Log.CloseAndFlush();
        }
    }
}

Serilog.Sinks.Slack version is 2.2.2. Serilog.Sinks.PeriodicBatching version is 5.0.0

A suggested solution would look something similar to the following code:

public class SlackMessageFormatter
{
    private readonly SlackSinkOptions _options;
    private readonly ITextFormatter _textFormatter;

    public SlackMessageFormatter(SlackSinkOptions options, ITextFormatter textFormatter)
    {
        _options = options;
        _textFormatter = textFormatter;
    }

    public Message CreateMessage(LogEvent logEvent)
    {
        using (var textWriter = new StringWriter())
        {
            _textFormatter.Format(logEvent, textWriter);
            return new Message
            {
                Text = textWriter.ToString(),
                // Set other properties...
            };
        }
    }

    // Add methods for creating attachments, etc.
}

public class SlackClient : IDisposable
{
    private readonly HttpClient _httpClient;

    public SlackClient()
    {
        _httpClient = new HttpClient();
    }

    public async Task SendMessageAsync(string webhookUrl, Message message)
    {
        var json = JsonConvert.SerializeObject(message);
        await _httpClient.PostAsync(webhookUrl, new StringContent(json));
    }

    public void Dispose()
    {
        _httpClient.Dispose();
    }
}

public class SlackBatchingSink : IBatchedLogEventSink, IDisposable
{
    private readonly SlackMessageFormatter _formatter;
    private readonly SlackClient _client;
    private readonly SlackSinkOptions _options;

    public SlackBatchingSink(SlackMessageFormatter formatter, SlackClient client, SlackSinkOptions options)
    {
        _formatter = formatter;
        _client = client;
        _options = options;
    }

    public async Task EmitBatchAsync(IEnumerable<LogEvent> events)
    {
        foreach (var logEvent in events)
        {
            var message = _formatter.CreateMessage(logEvent);
            await _client.SendMessageAsync(_options.WebHookUrl, message);
        }
    }

    public Task OnEmptyBatchAsync()
    {
        return Task.CompletedTask;
    }

    public void Dispose()
    {
        _client.Dispose();
    }
}

public class SlackSink : ILogEventSink, IDisposable
{
    private readonly PeriodicBatchingSink _periodicBatchingSink;

    public SlackSink(SlackSinkOptions options, ITextFormatter textFormatter)
    {
        var formatter = new SlackMessageFormatter(options, textFormatter);
        var client = new SlackClient();
        var batchingSink = new SlackBatchingSink(formatter, client, options);

        _periodicBatchingSink = new PeriodicBatchingSink(batchingSink, options.ToPeriodicBatchingSinkOptions());
    }

    public void Emit(LogEvent logEvent)
    {
        _periodicBatchingSink.Emit(logEvent);
    }

    public void Dispose()
    {
        _periodicBatchingSink.Dispose();
    }
}

Benefits of the Redesign:

Each class now has a single responsibility, making the code easier to understand, maintain, and test.

The SlackSink no longer directly manages both its disposal and that of its components. The components (like SlackClient and PeriodicBatchingSink) handle their own lifecycles independently, reducing the risk of recursive disposal calls.

The SlackMessageFormatter and SlackClient can be reused or tested independently, providing more flexibility in the codebase. Simplified Debugging:

If a stack overflow or other issue does occur, it’s easier to isolate the problem to a specific component, as each class now handles a distinct part of the process.

This redesign should help eliminate the stack overflow problem while also improving the overall architecture and maintainability of your logging infrastructure.

[TrapperHell]

Hi there. Thank you so much for your detailed response and analysis. This issue has already been reported as #48, but I appreciate how detailed this is.

I have actually been working on a revamp of the whole project removing the dependency on Newtonsoft.Json entirely, along with the PeriodicBatchingSink - which is now part of the core Serilog.

With that being said, I'm still debating how to proceed with versioning. I'm of the opinion that I will release one more version to at least fix the issue reported here, and then release a new major version which does away with the above-mentioned dependencies while also ending support for .net standard 1.*

[martin-pronestor]

Thanks for the update! It’s exciting to hear about the revamp, especially dropping Newtonsoft.Json and integrating the PeriodicBatchingSink directly into Serilog—those changes are bound to streamline things.

I think your plan for versioning makes a lot of sense. Pushing out one last version to address the current issue before rolling out the major overhaul seems like a smart way to keep things stable for users while you work on the bigger improvements. And yeah, ending support for .NET Standard 1.* feels like the right move given where things are headed.

I’m really looking forward to seeing what you come up with next! If there’s anything else I can help with or test, just let me know.