Response to review of #21

Response to review of #21

Author: cocowalla

src/Serilog.Sinks.Async/LoggerConfigurationAsyncExtensions.cs

@@ -1,6 +1,6 @@
 using System;
+using System.ComponentModel;
 using Serilog.Configuration;
-
 using Serilog.Sinks.Async;
 
 namespace Serilog
@@ -10,6 +10,24 @@ namespace Serilog
     /// </summary>
     public static class LoggerConfigurationAsyncExtensions
     {
+        /// <summary>
+        /// Configure a sink to be invoked asynchronously, on a background worker thread.
+        /// </summary>
+        /// <param name="loggerSinkConfiguration">The <see cref="LoggerSinkConfiguration"/> being configured.</param>
+        /// <param name="configure">An action that configures the wrapped sink.</param>
+        /// <param name="bufferSize">The size of the concurrent queue used to feed the background worker thread. If
+        /// the thread is unable to process events quickly enough and the queue is filled, subsequent events will be 
+        /// dropped until room is made in the queue.</param>
+        /// <returns>A <see cref="LoggerConfiguration"/> allowing configuration to continue.</returns>
+        [EditorBrowsable(EditorBrowsableState.Never)]
+        public static LoggerConfiguration Async(
+            this LoggerSinkConfiguration loggerSinkConfiguration,
+            Action<LoggerSinkConfiguration> configure,
+            int bufferSize)
+        {
+            return loggerSinkConfiguration.Async(configure, bufferSize, false);
+        }
+
         /// <summary>
         /// Configure a sink to be invoked asynchronously, on a background worker thread.
         /// </summary>
@@ -32,5 +50,6 @@ public static LoggerConfiguration Async(
                 wrappedSink => new BackgroundWorkerSink(wrappedSink, bufferSize, blockWhenFull),
                 configure);
         }
+
     }
 }

src/Serilog.Sinks.Async/Sinks/Async/BackgroundWorkerSink.cs

@@ -13,7 +13,6 @@ sealed class BackgroundWorkerSink : ILogEventSink, IDisposable
         readonly ILogEventSink _pipeline;
         readonly int _bufferCapacity;
         readonly bool _blockWhenFull;
-        volatile bool _disposed;
         readonly BlockingCollection<LogEvent> _queue;
         readonly Task _worker;
 
@@ -30,27 +29,36 @@ public BackgroundWorkerSink(ILogEventSink pipeline, int bufferCapacity, bool blo
 
         public void Emit(LogEvent logEvent)
         {
-            // The disposed check is racy, but only ensures we don't prevent flush from
-            // completing by pushing more events.
-            if (_disposed)
+            if (this._queue.IsAddingCompleted)
                 return;
 
-            if (!this._blockWhenFull)
+            try
             {
-                if (!_queue.TryAdd(logEvent))
-                    SelfLog.WriteLine("{0} unable to enqueue, capacity {1}", typeof(BackgroundWorkerSink), _bufferCapacity);
+                if (_blockWhenFull)
+                {
+                    _queue.Add(logEvent);
+                }
+                else
+                {
+                    if (!_queue.TryAdd(logEvent))
+                        SelfLog.WriteLine("{0} unable to enqueue, capacity {1}", typeof(BackgroundWorkerSink), _bufferCapacity);
+                }
             }
-            else
+            catch (InvalidOperationException)
             {
-                this._queue.Add(logEvent);
+                // Thrown in the event of a race condition when we try to add another event after 
+                // CompleteAdding has been called
             }
         }
 
         public void Dispose()
         {
-            _disposed = true;
+            // Prevent any more events from being added
             _queue.CompleteAdding();
-            _worker.Wait();            
+
+            // Allow queued events to be flushed
+            _worker.Wait();      
+            
             (_pipeline as IDisposable)?.Dispose();
         }
 

test/Serilog.Sinks.Async.Tests/BackgroundWorkerSinkSpec.cs

@@ -4,7 +4,6 @@
 using System.Linq;
 using System.Threading.Tasks;
 using Serilog.Core;
-using Serilog.Debugging;
 using Serilog.Events;
 using Serilog.Parsing;
 using Serilog.Sinks.Async.Tests.Support;
@@ -41,7 +40,6 @@ public async Task WhenEmitSingle_ThenRelaysToInnerSink()
         {
             var logEvent = CreateEvent();
             _sink.Emit(logEvent);
-            _sink.Dispose();
 
             await Task.Delay(TimeSpan.FromSeconds(3));
 
@@ -90,7 +88,7 @@ public async Task WhenQueueFull_ThenDropsEvents()
 
             // Cause a delay when emmitting to the inner sink, allowing us to fill the queue to capacity 
             // after the first event is popped
-            _innerSink.DelayEmit = true;
+            _innerSink.DelayEmit = TimeSpan.FromMilliseconds(300);
 
             var events = new List<LogEvent>
             {
@@ -106,12 +104,12 @@ public async Task WhenQueueFull_ThenDropsEvents()
                 _sink.Emit(e);
                 sw.Stop();
 
-                Assert.True(sw.ElapsedMilliseconds < 2000, "Should not block the caller when the queue is full");
+                Assert.True(sw.ElapsedMilliseconds < 200, "Should not block the caller when the queue is full");
             });
 
             // If we *weren't* dropped events, the delay in the inner sink would mean the 5 events would take 
             // at least 15 seconds to process
-            await Task.Delay(TimeSpan.FromSeconds(18));
+            await Task.Delay(TimeSpan.FromSeconds(2));
 
             // Events should be dropped
             Assert.Equal(2, _innerSink.Events.Count);
@@ -124,7 +122,7 @@ public async Task WhenQueueFull_ThenBlocks()
 
             // Cause a delay when emmitting to the inner sink, allowing us to fill the queue to capacity 
             // after the first event is popped
-            _innerSink.DelayEmit = true;
+            _innerSink.DelayEmit = TimeSpan.FromMilliseconds(300);
 
             var events = new List<LogEvent>
             {
@@ -144,11 +142,11 @@ public async Task WhenQueueFull_ThenBlocks()
                 // subsequent calls, the queue should be full, so we should be blocked
                 if (i > 0)
                 {
-                    Assert.True(sw.ElapsedMilliseconds > 2000, "Should block the caller when the queue is full");
+                    Assert.True(sw.ElapsedMilliseconds > 200, "Should block the caller when the queue is full");
                 }
             });
 
-            await Task.Delay(TimeSpan.FromSeconds(12));
+            await Task.Delay(TimeSpan.FromSeconds(2));
 
             // No events should be dropped
             Assert.Equal(3, _innerSink.Events.Count);

test/Serilog.Sinks.Async.Tests/Support/MemorySink.cs

@@ -10,12 +10,12 @@ public class MemorySink : ILogEventSink
     {
         public ConcurrentBag<LogEvent> Events { get; } = new ConcurrentBag<LogEvent>();
         public bool ThrowAfterCollecting { get; set; }
-        public bool DelayEmit { get; set; }
+        public TimeSpan? DelayEmit { get; set; }
 
         public void Emit(LogEvent logEvent)
         {
-            if (DelayEmit)
-                Task.Delay(TimeSpan.FromSeconds(3)).Wait();
+            if (DelayEmit.HasValue)
+                Task.Delay(DelayEmit.Value).Wait();
 
             Events.Add(logEvent);