serilog / serilog-sinks-periodicbatching

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

PeriodicBatchingSink ctor fails when NoQueueLimit is used as documented #45

Closed erdalsivri closed 4 years ago

erdalsivri commented 4 years ago

PeriodicBatchingSink has a constructor to specify queueLimit, which has a documentation that says:

Maximum number of events in the queue - use <see cref="F:Serilog.Sinks.PeriodicBatching.PeriodicBatchingSink.NoQueueLimit" /> for an unbounded queue.

However, setting NoQueueLimit, which is equal to -1, doesn't work because BoundedConcurrentQueue fails when queueLimit is less than zero:

public BoundedConcurrentQueue(int? queueLimit = null)
{
  if (queueLimit.HasValue && queueLimit <= 0)
    throw new ArgumentOutOfRangeException(nameof(queueLimit), "Queue limit must be positive, or `null` to indicate unbounded.");

   _queueLimit = queueLimit ?? Unbounded;
}

PeriodicBatchingSink should either make the queueLimit parameter nullable and change NoQueueLimit to be null or Initialize PeriodicBatchingSinkOptions.QueueLimit conditionally like this:

QueueLimit = queueLimit == NoQueueLimit ? new int?() : new int?(queueLimit)

Alternative is to fix BoundedConcurrentQueue to accept BoundedConcurrentQueue.Unbounded (-1) as unlimited instead of failing the precondition check. This wouldn't be very nice because then there would be two ways of specifying unbounded: null and -1.

nblumhardt commented 4 years ago

Thank you for the report; submitted #46 with the fix :+1:

Heads-up, if you're maintaining a sink that depends upon this one, check out the newer non-subclassing API and PeriodicBatchingSinkOptions - example's now in https://github.com/serilog/serilog-sinks-periodicbatching#getting-started

Cheers!

erdalsivri commented 4 years ago

Thanks for the very fast fix!