Merge pull request #10 from nblumhardt/threadsafe-context

Make DiagnosticContextCollector thread-safe

README.md

@@ -1,6 +1,6 @@
 # Serilog.Extensions.Hosting [![Build status](https://ci.appveyor.com/api/projects/status/ue4s7htjwj88fulh?svg=true)](https://ci.appveyor.com/project/serilog/serilog-extensions-hosting) [![NuGet Version](http://img.shields.io/nuget/v/Serilog.Extensions.Hosting.svg?style=flat)](https://www.nuget.org/packages/Serilog.Extensions.Hosting/) 
 
-Serilog logging for Microsoft.Extensions.Hosting . This package routes Microsoft.Extensions.Hosting log messages through Serilog, so you can get information about the framework's internal operations logged to the same Serilog sinks as your application events.
+Serilog logging for _Microsoft.Extensions.Hosting_. This package routes framework log messages through Serilog, so you can get information about the framework's internal operations written to the same Serilog sinks as your application events.
 
 ### Instructions
 

src/Serilog.Extensions.Hosting/Extensions/Hosting/AmbientDiagnosticContextCollector.cs

@@ -1,4 +1,18 @@
-using System;
+// Copyright 2019 Serilog Contributors
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+using System;
 using System.Threading;
 
 namespace Serilog.Extensions.Hosting

src/Serilog.Extensions.Hosting/Extensions/Hosting/DiagnosticContext.cs

@@ -13,14 +13,15 @@
 // limitations under the License.
 
 using System;
+using System.Threading;
 
 namespace Serilog.Extensions.Hosting
 {
     /// <summary>
-    /// Implements an ambient/async-local diagnostic context. Consumers should
-    /// use <see cref="IDiagnosticContext"/> in preference to this concrete type.
+    /// Implements an ambient diagnostic context using <see cref="AsyncLocal{T}"/>.
     /// </summary>
-    public class DiagnosticContext : IDiagnosticContext
+    /// <remarks>Consumers should use <see cref="IDiagnosticContext"/> to set context properties.</remarks>
+    public sealed class DiagnosticContext : IDiagnosticContext
     {
         readonly ILogger _logger;
 
@@ -34,25 +35,25 @@ public DiagnosticContext(ILogger logger)
         }
 
         /// <summary>
-        /// Start collecting properties to associate with the current async context. This will replace
+        /// Start collecting properties to associate with the current diagnostic context. This will replace
         /// the active collector, if any.
         /// </summary>
-        /// <returns>A collector that will receive events added in the current async context.</returns>
+        /// <returns>A collector that will receive properties added in the current diagnostic context.</returns>
         public DiagnosticContextCollector BeginCollection()
         {
             return AmbientDiagnosticContextCollector.Begin();
         }
 
-        /// <inheritdoc cref="IDiagnosticContext.Add"/>
-        public void Add(string propertyName, object value, bool destructureObjects = false)
+        /// <inheritdoc cref="IDiagnosticContext.Set"/>
+        public void Set(string propertyName, object value, bool destructureObjects = false)
         {
             if (propertyName == null) throw new ArgumentNullException(nameof(propertyName));
 
             var collector = AmbientDiagnosticContextCollector.Current;
             if (collector != null && 
                 (_logger ?? Log.Logger).BindProperty(propertyName, value, destructureObjects, out var property))
             {
-                collector.Add(property);
+                collector.AddOrUpdate(property);
             }
         }
     }

src/Serilog.Extensions.Hosting/Extensions/Hosting/DiagnosticContextCollector.cs

@@ -9,22 +9,33 @@ namespace Serilog.Extensions.Hosting
     /// </summary>
     public sealed class DiagnosticContextCollector : IDisposable
     {
-        readonly AmbientDiagnosticContextCollector _ambientCollector;
-        List<LogEventProperty> _properties = new List<LogEventProperty>();
+        readonly IDisposable _chainedDisposable;
+        readonly object _propertiesLock = new object();
+        Dictionary<string, LogEventProperty> _properties = new Dictionary<string, LogEventProperty>();
 
-        internal DiagnosticContextCollector(AmbientDiagnosticContextCollector ambientCollector)
+        /// <summary>
+        /// Construct a <see cref="DiagnosticContextCollector"/>.
+        /// </summary>
+        /// <param name="chainedDisposable">An object that will be disposed to signal completion/disposal of
+        /// the collector.</param>
+        public DiagnosticContextCollector(IDisposable chainedDisposable)
         {
-            _ambientCollector = ambientCollector ?? throw new ArgumentNullException(nameof(ambientCollector));
+            _chainedDisposable = chainedDisposable ?? throw new ArgumentNullException(nameof(chainedDisposable));
         }
 
         /// <summary>
         /// Add the property to the context.
         /// </summary>
         /// <param name="property">The property to add.</param>
-        public void Add(LogEventProperty property)
+        public void AddOrUpdate(LogEventProperty property)
         {
             if (property == null) throw new ArgumentNullException(nameof(property));
-            _properties?.Add(property);
+
+            lock (_propertiesLock)
+            {
+                if (_properties == null) return;
+                _properties[property.Name] = property;
+            }
         }
 
         /// <summary>
@@ -34,18 +45,21 @@ public void Add(LogEventProperty property)
         /// </summary>
         /// <param name="properties">The collected properties, or null if no collection is active.</param>
         /// <returns>True if properties could be collected.</returns>
-        public bool TryComplete(out List<LogEventProperty> properties)
+        public bool TryComplete(out IEnumerable<LogEventProperty> properties)
         {
-            properties = _properties;
-            _properties = null;
-            Dispose();
-            return properties != null;
+            lock (_propertiesLock)
+            {
+                properties = _properties?.Values;
+                _properties = null;
+                Dispose();
+                return properties != null;
+            }
         }
 
         /// <inheritdoc/>
         public void Dispose()
         {
-            _ambientCollector.Dispose();
+            _chainedDisposable.Dispose();
         }
     }
 }

src/Serilog.Extensions.Hosting/IDiagnosticContext.cs

@@ -20,12 +20,13 @@ namespace Serilog
     public interface IDiagnosticContext
     {
         /// <summary>
-        /// Add the specified property to the request's diagnostic payload.
+        /// Set the specified property on the current diagnostic context. The property will be collected
+        /// and attached to the event emitted at the completion of the context.
         /// </summary>
         /// <param name="propertyName">The name of the property. Must be non-empty.</param>
         /// <param name="value">The property value.</param>
         /// <param name="destructureObjects">If true, the value will be serialized as structured
         /// data if possible; if false, the object will be recorded as a scalar or simple array.</param>
-        void Add(string propertyName, object value, bool destructureObjects = false); 
+        void Set(string propertyName, object value, bool destructureObjects = false); 
     }
 }

test/Serilog.Extensions.Hosting.Tests/DiagnosticContextTests.cs

@@ -1,17 +1,21 @@
 using System;
+using System.Linq;
 using System.Threading.Tasks;
+using Serilog.Events;
 using Serilog.Extensions.Hosting.Tests.Support;
 using Xunit;
 
+// ReSharper disable PossibleNullReferenceException
+
 namespace Serilog.Extensions.Hosting.Tests
 {
     public class DiagnosticContextTests
     {
         [Fact]
-        public void AddIsSafeWhenNoContextIsActive()
+        public void SetIsSafeWhenNoContextIsActive()
         {
             var dc = new DiagnosticContext(Some.Logger());
-            dc.Add(Some.String("name"), Some.Int32());
+            dc.Set(Some.String("name"), Some.Int32());
         }
 
         [Fact]
@@ -20,19 +24,34 @@ public async Task PropertiesAreCollectedInAnActiveContext()
             var dc = new DiagnosticContext(Some.Logger());
 
             var collector = dc.BeginCollection();
-            dc.Add(Some.String("name"), Some.Int32());
+            dc.Set(Some.String("first"), Some.Int32());
             await Task.Delay(TimeSpan.FromMilliseconds(10));
-            dc.Add(Some.String("name"), Some.Int32());
+            dc.Set(Some.String("second"), Some.Int32());
 
             Assert.True(collector.TryComplete(out var properties));
-            Assert.Equal(2, properties.Count);
+            Assert.Equal(2, properties.Count());
 
             Assert.False(collector.TryComplete(out _));
 
             collector.Dispose();
 
-            dc.Add(Some.String("name"), Some.Int32());
+            dc.Set(Some.String("third"), Some.Int32());
             Assert.False(collector.TryComplete(out _));
         }
+
+        [Fact]
+        public void ExistingPropertiesCanBeUpdated()
+        {
+            var dc = new DiagnosticContext(Some.Logger());
+
+            var collector = dc.BeginCollection();
+            dc.Set("name", 10);
+            dc.Set("name", 20);
+
+            Assert.True(collector.TryComplete(out var properties));
+            var prop = Assert.Single(properties);
+            var scalar = Assert.IsType<ScalarValue>(prop.Value);
+            Assert.Equal(20, scalar.Value);
+        }
     }
 }