AspNetSessionValueLayoutRenderer - Skip ReEntrantScopeLock for classi…

…c NLog.Web

src/Shared/Internal/ReEntrantScopeLock.cs

@@ -1,79 +1,27 @@
-using System;
 #if !NET35
-using System.Threading;
-#endif
-#if ASP_NET_CORE
-using HttpContextBase = Microsoft.AspNetCore.Http.HttpContext;
-#else
-using System.Web;
-#endif
 
 namespace NLog.Web.Internal
 {
+    using System;
+    using System.Threading;
+
     /// <summary>
-    /// Manages if a LayoutRenderer can be called recursively.
-    /// Especially used by AspNetSessionValueLayoutRenderer
-    /// 
-    /// Since NET 35 does not support AsyncLocal or even ThreadLocal
-    /// a different technique must be used for that platform
+    /// Manages if a LayoutRenderer can be called recursively using AsyncLocal
+    /// Example used by <see cref="NLog.Web.LayoutRenderers.AspNetSessionValueLayoutRenderer"/>
     /// </summary>
     internal readonly struct ReEntrantScopeLock : IDisposable
     {
-        internal ReEntrantScopeLock(HttpContextBase context)
+        public ReEntrantScopeLock(bool acquireLock = true)
         {
-            _httpContext = context;
-            IsLockAcquired = TryGetLock(context);
+            IsLockAcquired = acquireLock && TryGetLock();
         }
 
-        private readonly HttpContextBase _httpContext;
-
-        // Need to track if we were successful in the lock
-        // If we were not, we should not unlock in the dispose code
         internal bool IsLockAcquired { get; }
 
-#if NET35
-        private static readonly object ReEntrantLock = new object();
-
-        private static bool TryGetLock(HttpContextBase context)
-        {
-            // If context is null leave
-            if (context == null)
-            {
-                return false;
-            }
-
-            // If already locked, return false
-            if (context.Items?.Contains(ReEntrantLock) == true)
-            {
-                return false;
-            }
-
-            // Get the lock
-            context.Items[ReEntrantLock] = bool.TrueString;
-
-            // Indicate the lock was successfully acquired
-            return true;
-        }
-
-        public void Dispose()
-        {
-            // Only unlock if we were the ones who locked it
-            if (IsLockAcquired)
-            {
-                _httpContext.Items?.Remove(ReEntrantLock);
-            }
-        }
-#else
         private static readonly AsyncLocal<bool> ReEntrantLock = new AsyncLocal<bool>();
 
-        private static bool TryGetLock(HttpContextBase context)
+        private static bool TryGetLock()
         {
-            // If context is null leave
-            if (context == null)
-            {
-                return false;
-            }
-
             // If already locked, return false
             if (ReEntrantLock.Value)
             {
@@ -95,6 +43,8 @@ public void Dispose()
                 ReEntrantLock.Value = false;
             }
         }
-#endif
     }
 }
+
+
+#endif

src/Shared/LayoutRenderers/AspNetSessionIdLayoutRenderer.cs

@@ -1,4 +1,5 @@
 using System.Text;
+using NLog.Common;
 using NLog.Config;
 using NLog.LayoutRenderers;
 using NLog.Web.Internal;
@@ -22,15 +23,31 @@ public class AspNetSessionIdLayoutRenderer : AspNetLayoutRendererBase
         /// <inheritdoc/>
         protected override void DoAppend(StringBuilder builder, LogEventInfo logEvent)
         {
-            var contextSession = HttpContextAccessor.HttpContext.TryGetSession();
-            if (contextSession == null)
-                return;
+#if ASP_NET_CORE
+            // Because session.get / session.getstring are also creating log messages in some cases,
+            //  this could lead to stack overflow issues. 
+            // We remember that we are looking up a session value so we prevent stack overflows
+            using (var reEntryScopeLock = new ReEntrantScopeLock(true))
+            {
+                if (!reEntryScopeLock.IsLockAcquired)
+                {
+                    InternalLogger.Debug("aspnet-session-item - Lookup skipped because reentrant-scope-lock already taken");
+                    return;
+                }
+#else
+            {
+#endif
+                var contextSession = HttpContextAccessor.HttpContext.TryGetSession();
+                if (contextSession == null)
+                    return;
+
 
 #if !ASP_NET_CORE
-            builder.Append(contextSession.SessionID);
+                builder.Append(contextSession.SessionID);
 #else
-            builder.Append(contextSession.Id);
+                builder.Append(contextSession.Id);
 #endif
+            }
         }
     }
 }

src/Shared/LayoutRenderers/AspNetSessionValueLayoutRenderer.cs

@@ -1,13 +1,15 @@
+using System;
 using System.Globalization;
 using System.Text;
+using NLog.Common;
 using NLog.Config;
 using NLog.LayoutRenderers;
 using NLog.Web.Internal;
-using NLog.Common;
 #if ASP_NET_CORE
 using Microsoft.AspNetCore.Http;
 #else
 using System.Web;
+using ISession = System.Web.HttpSessionStateBase;
 #endif
 
 namespace NLog.Web.LayoutRenderers
@@ -76,9 +78,23 @@ public class AspNetSessionValueLayoutRenderer : AspNetLayoutRendererBase
         /// <summary>
         /// The type of the value.
         /// </summary>
-        public SessionValueType ValueType { get; set; } = SessionValueType.String;
+        public SessionValueType ValueType
+        {
+            get => _valueType;
+            set
+            {
+                _valueType = value;
+                if (value == SessionValueType.Int32)
+                    _sessionValueLookup = (session, key) => GetSessionIntValue(session, key);
+                else
+                    _sessionValueLookup = (session, key) => GetSessionValue(session, key);
+            }
+        }
+        private SessionValueType _valueType;
 #endif
 
+        private Func<ISession, string, object> _sessionValueLookup = (session, key) => GetSessionValue(session, key);   // Skip delegate allocation for ValueType
+
         /// <inheritdoc/>
         protected override void DoAppend(StringBuilder builder, LogEventInfo logEvent)
         {
@@ -94,26 +110,28 @@ protected override void DoAppend(StringBuilder builder, LogEventInfo logEvent)
                 return;
             }
 
-            var contextSession = context?.TryGetSession();
-            if (contextSession == null)
-            {
-                return;
-            }
-
+#if ASP_NET_CORE
             // Because session.get / session.getstring are also creating log messages in some cases,
-            //   this could lead to stack overflow issues. 
+            //  this could lead to stack overflow issues. 
             // We remember that we are looking up a session value so we prevent stack overflows
-            using (var reEntry = new ReEntrantScopeLock(context))
+            using (var reEntryScopeLock = new ReEntrantScopeLock(true))
             {
-                if (!reEntry.IsLockAcquired)
+                if (!reEntryScopeLock.IsLockAcquired)
                 {
-                    InternalLogger.Debug(
-                        "Reentrant log event detected. Logging when inside the scope of another log event can cause a StackOverflowException.");
+                    InternalLogger.Debug("aspnet-session-item - Lookup skipped because reentrant-scope-lock already taken");
                     return;
                 }
+#else
+            {
+#endif
 
-                var value = PropertyReader.GetValue(item, contextSession, GetSessionValue, EvaluateAsNestedProperties);
+                var contextSession = context?.TryGetSession();
+                if (contextSession == null)
+                {
+                    return;
+                }
 
+                var value = PropertyReader.GetValue(item, contextSession, _sessionValueLookup, EvaluateAsNestedProperties);
                 if (value != null)
                 {
                     var formatProvider = GetFormatProvider(logEvent, Culture);
@@ -123,18 +141,18 @@ protected override void DoAppend(StringBuilder builder, LogEventInfo logEvent)
         }
 
 #if ASP_NET_CORE
-        private object GetSessionValue(ISession session, string key)
+        private static object GetSessionIntValue(ISession session, string key)
         {
-            switch (ValueType)
-            {
-                case SessionValueType.Int32:
-                    return session.GetInt32(key);
-                default: 
-                    return session.GetString(key);
-            }
+            var value = session.GetInt32(key);
+            return value.HasValue ? (object)value.Value : null;
+        }
+
+        private static object GetSessionValue(ISession session, string key)
+        {
+            return session.GetString(key);
         }
 #else
-        private object GetSessionValue(HttpSessionStateBase session, string key)
+        private static object GetSessionValue(ISession session, string key)
         {
             return session.Count == 0 ? null : session[key];
         }