From 0a309a4f342327e900ada2d01f91dc37cd744428 Mon Sep 17 00:00:00 2001 From: Rolf Kristensen Date: Fri, 12 Aug 2022 22:00:30 +0200 Subject: [PATCH] AspNetSessionValueLayoutRenderer - Skip ReEntrantScopeLock for classic NLog.Web --- src/Shared/Internal/ReEntrantScopeLock.cs | 72 +++---------------- .../AspNetSessionIdLayoutRenderer.cs | 27 +++++-- .../AspNetSessionValueLayoutRenderer.cs | 64 +++++++++++------ 3 files changed, 74 insertions(+), 89 deletions(-) diff --git a/src/Shared/Internal/ReEntrantScopeLock.cs b/src/Shared/Internal/ReEntrantScopeLock.cs index 67bf30c7..5e13f701 100644 --- a/src/Shared/Internal/ReEntrantScopeLock.cs +++ b/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; + /// - /// 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 /// 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 ReEntrantLock = new AsyncLocal(); - 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 \ No newline at end of file diff --git a/src/Shared/LayoutRenderers/AspNetSessionIdLayoutRenderer.cs b/src/Shared/LayoutRenderers/AspNetSessionIdLayoutRenderer.cs index 20fa514a..6679b870 100644 --- a/src/Shared/LayoutRenderers/AspNetSessionIdLayoutRenderer.cs +++ b/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 /// 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 + } } } } \ No newline at end of file diff --git a/src/Shared/LayoutRenderers/AspNetSessionValueLayoutRenderer.cs b/src/Shared/LayoutRenderers/AspNetSessionValueLayoutRenderer.cs index dbde34e9..4ba9f59d 100644 --- a/src/Shared/LayoutRenderers/AspNetSessionValueLayoutRenderer.cs +++ b/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 /// /// The type of the value. /// - 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 _sessionValueLookup = (session, key) => GetSessionValue(session, key); // Skip delegate allocation for ValueType + /// 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]; }