serilog / serilog-sinks-periodicbatching

Infrastructure for Serilog sinks that process events in batches.
Apache License 2.0
70 stars 29 forks source link

Calling Dispose() causes an infinite loop #73

Closed wparad closed 2 months ago

wparad commented 3 months ago

It isn't clear how dispose is even supposed to be called. Realistically the consumers of an IBatchedLogEventSink implementation will cause Dispose on the IBatchedLogEventSink, and that means the IBatchedLogEventSink implementation needs to call Dispose on the PeriodicBatchingSink.

But! Then the PeriodicBatchingSink calls Dispose on the IBatchedLogEventSink, which creates a recursive loop. The alternative:

IBatchedLogEventSink never calls Dispose - Well then Dispose never gets called on the PeriodicBatchingSink which is wrong.

That leads me to believe that this is a bug In Dispose() and it never should call back Dispose on the IBatchedLogEventSink which is already in the process of being called.

The fix is remove the IBatchedLogEventSink.Dispose() call. Before we go and do that, can we explain why this bug might exist and what the expected implementation is supposed to be.

Original Find: https://github.com/Cimpress-MCP/serilog-sinks-awscloudwatch/issues/133

nblumhardt commented 3 months ago

Thanks for the note. This is a bug in my PR to Serilog.Sinks.AwsCloudWatch - I've included some explanation in the fix PR https://github.com/Cimpress-MCP/serilog-sinks-awscloudwatch/pull/134

wparad commented 3 months ago

Would it makes sense to add some protection to the Dispose method like was previously there (in the ~3.0.0 version) to prevent other implementations from running into the same problem?

nblumhardt commented 3 months ago

Seems like one to keep an eye on 👍