From 99b5345718f9128ab970c0e113c342e5c510d418 Mon Sep 17 00:00:00 2001 From: Burak Akgerman Date: Wed, 10 Aug 2022 11:05:30 -0400 Subject: [PATCH 01/18] Re-Entrancy management for Session Value Layout Render. Also fix spelling errors and remove unused using statements. --- .../AspNetSessionValueLayoutRenderer.cs | 56 ++++++++++--------- 1 file changed, 31 insertions(+), 25 deletions(-) diff --git a/src/Shared/LayoutRenderers/AspNetSessionValueLayoutRenderer.cs b/src/Shared/LayoutRenderers/AspNetSessionValueLayoutRenderer.cs index 9774f0e4..78d22a55 100644 --- a/src/Shared/LayoutRenderers/AspNetSessionValueLayoutRenderer.cs +++ b/src/Shared/LayoutRenderers/AspNetSessionValueLayoutRenderer.cs @@ -1,12 +1,11 @@ -using System; using System.Globalization; using System.Text; +using System.Threading; using NLog.Config; using NLog.LayoutRenderers; using NLog.Web.Internal; -#if !ASP_NET_CORE -using System.Web; -#else +using NLog.Common; +#if ASP_NET_CORE using Microsoft.AspNetCore.Http; #endif @@ -40,10 +39,6 @@ namespace NLog.Web.LayoutRenderers [LayoutRenderer("aspnet-session")] public class AspNetSessionValueLayoutRenderer : AspNetLayoutRendererBase { -#if ASP_NET_CORE - private static readonly object NLogRetrievingSessionValue = new object(); -#endif - /// /// Gets or sets the session item name. /// @@ -78,11 +73,15 @@ public class AspNetSessionValueLayoutRenderer : AspNetLayoutRendererBase #if ASP_NET_CORE /// - /// The hype of the value. + /// The type of the value. /// public SessionValueType ValueType { get; set; } = SessionValueType.String; #endif +#if !NET35 + // Manage access to the session re-entrancy, at least above .NET 3.5 + private static readonly AsyncLocal IsReEntrant = new AsyncLocal(); +#endif /// protected override void DoAppend(StringBuilder builder, LogEventInfo logEvent) { @@ -93,39 +92,45 @@ protected override void DoAppend(StringBuilder builder, LogEventInfo logEvent) } var context = HttpContextAccessor.HttpContext; - var contextSession = context.TryGetSession(); + var contextSession = context?.TryGetSession(); if (contextSession == null) + { return; - -#if !ASP_NET_CORE - var value = PropertyReader.GetValue(item, contextSession, (session,key) => session.Count > 0 ? session[key] : null, EvaluateAsNestedProperties); -#else - //because session.get / session.getstring also creating log messages in some cases, this could lead to stackoverflow issues. - //We remember on the context.Items that we are looking up a session value so we prevent stackoverflows - if (context.Items == null || (context.Items.Count > 0 && context.Items.ContainsKey(NLogRetrievingSessionValue))) + } +#if !NET35 + // If we are already in this layout render in the same async path, we should stop the recursion + if (IsReEntrant.Value) { + InternalLogger.Error($"Reentrant log event detected. Logging when inside the scope of another log event can cause a StackOverflowException. LogEventInfo.Message:{logEvent.Message}"); return; } - - context.Items[NLogRetrievingSessionValue] = bool.TrueString; - + // Mark that we have entered the session + IsReEntrant.Value = true; +#endif + // Perform the PropertyReader.GetValue() in a try/finally clause since we want to set the IsReEntrant to false even if there is an Exception object value; try { - value = PropertyReader.GetValue(item, contextSession, (session, key) => GetSessionValue(session, key), EvaluateAsNestedProperties); +#if !ASP_NET_CORE + value = PropertyReader.GetValue(item, contextSession, + (session,key) => session.Count > 0 ? session[key] : null, EvaluateAsNestedProperties); +#else + value = PropertyReader.GetValue(item, contextSession, GetSessionValue, EvaluateAsNestedProperties); +#endif } finally { - context.Items.Remove(NLogRetrievingSessionValue); - } +#if !NET35 + IsReEntrant.Value = false; #endif + } + if (value != null) { var formatProvider = GetFormatProvider(logEvent, Culture); builder.AppendFormattedValue(value, Format, formatProvider, ValueFormatter); } } - #if ASP_NET_CORE private object GetSessionValue(ISession session, string key) { @@ -133,7 +138,8 @@ private object GetSessionValue(ISession session, string key) { case SessionValueType.Int32: return session.GetInt32(key); - default: return session.GetString(key); + default: + return session.GetString(key); } } #endif From 430e139f75040efc6b97e5ab9528e73f5306d0ce Mon Sep 17 00:00:00 2001 From: Burak Akgerman Date: Wed, 10 Aug 2022 13:13:34 -0400 Subject: [PATCH 02/18] Make the code more similar between ASP.NET and ASP.NET Core. --- .../AspNetSessionValueLayoutRenderer.cs | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/src/Shared/LayoutRenderers/AspNetSessionValueLayoutRenderer.cs b/src/Shared/LayoutRenderers/AspNetSessionValueLayoutRenderer.cs index 78d22a55..8467a060 100644 --- a/src/Shared/LayoutRenderers/AspNetSessionValueLayoutRenderer.cs +++ b/src/Shared/LayoutRenderers/AspNetSessionValueLayoutRenderer.cs @@ -7,6 +7,8 @@ using NLog.Common; #if ASP_NET_CORE using Microsoft.AspNetCore.Http; +#else +using System.Web; #endif namespace NLog.Web.LayoutRenderers @@ -111,12 +113,7 @@ protected override void DoAppend(StringBuilder builder, LogEventInfo logEvent) object value; try { -#if !ASP_NET_CORE - value = PropertyReader.GetValue(item, contextSession, - (session,key) => session.Count > 0 ? session[key] : null, EvaluateAsNestedProperties); -#else value = PropertyReader.GetValue(item, contextSession, GetSessionValue, EvaluateAsNestedProperties); -#endif } finally { @@ -142,6 +139,11 @@ private object GetSessionValue(ISession session, string key) return session.GetString(key); } } +#else + private object GetSessionValue(HttpSessionStateBase session, string key) + { + return session.Count == 0 ? null : session[key]; + } #endif } } \ No newline at end of file From 60e83296730f20e01532fc425a0b242ffab995c1 Mon Sep 17 00:00:00 2001 From: Burak Akgerman Date: Wed, 10 Aug 2022 15:33:22 -0400 Subject: [PATCH 03/18] Add NET 35 recursion prevention --- .../AspNetSessionValueLayoutRenderer.cs | 26 ++++++++++++++++++- 1 file changed, 25 insertions(+), 1 deletion(-) diff --git a/src/Shared/LayoutRenderers/AspNetSessionValueLayoutRenderer.cs b/src/Shared/LayoutRenderers/AspNetSessionValueLayoutRenderer.cs index 8467a060..83a731f8 100644 --- a/src/Shared/LayoutRenderers/AspNetSessionValueLayoutRenderer.cs +++ b/src/Shared/LayoutRenderers/AspNetSessionValueLayoutRenderer.cs @@ -83,6 +83,9 @@ public class AspNetSessionValueLayoutRenderer : AspNetLayoutRendererBase #if !NET35 // Manage access to the session re-entrancy, at least above .NET 3.5 private static readonly AsyncLocal IsReEntrant = new AsyncLocal(); +#else + // NET 35 does not have AsyncLocal or even older ThreadLocal + private static readonly object IsReEntrant = new object(); #endif /// protected override void DoAppend(StringBuilder builder, LogEventInfo logEvent) @@ -94,13 +97,18 @@ protected override void DoAppend(StringBuilder builder, LogEventInfo logEvent) } var context = HttpContextAccessor.HttpContext; + if (context == null) + { + return; + } + var contextSession = context?.TryGetSession(); if (contextSession == null) { return; } #if !NET35 - // If we are already in this layout render in the same async path, we should stop the recursion + // If we are already in this layout render in the same path, we should stop the recursion if (IsReEntrant.Value) { InternalLogger.Error($"Reentrant log event detected. Logging when inside the scope of another log event can cause a StackOverflowException. LogEventInfo.Message:{logEvent.Message}"); @@ -108,6 +116,20 @@ protected override void DoAppend(StringBuilder builder, LogEventInfo logEvent) } // Mark that we have entered the session IsReEntrant.Value = true; +#else + if (context.Items == null) + { + return; + } + + // If we are already in this layout render in the same path, we should stop the recursion + if (context.Items.Contains(IsReEntrant)) + { + InternalLogger.Error($"Reentrant log event detected. Logging when inside the scope of another log event can cause a StackOverflowException. LogEventInfo.Message:{logEvent.Message}"); + return; + } + // Mark that we have entered the session + context.Items[IsReEntrant] = bool.TrueString; #endif // Perform the PropertyReader.GetValue() in a try/finally clause since we want to set the IsReEntrant to false even if there is an Exception object value; @@ -119,6 +141,8 @@ protected override void DoAppend(StringBuilder builder, LogEventInfo logEvent) { #if !NET35 IsReEntrant.Value = false; +#else + context.Items.Remove(IsReEntrant); #endif } From 308439000d26f069581e86c4d5144205fcf0d8da Mon Sep 17 00:00:00 2001 From: Burak Akgerman Date: Thu, 11 Aug 2022 11:39:43 -0400 Subject: [PATCH 04/18] Implement a re-entrancy manager --- .../Internal/RendererReEntrantManager.cs | 120 ++++++++++++++++++ .../AspNetSessionValueLayoutRenderer.cs | 66 +++------- 2 files changed, 139 insertions(+), 47 deletions(-) create mode 100644 src/Shared/Internal/RendererReEntrantManager.cs diff --git a/src/Shared/Internal/RendererReEntrantManager.cs b/src/Shared/Internal/RendererReEntrantManager.cs new file mode 100644 index 00000000..e9deb087 --- /dev/null +++ b/src/Shared/Internal/RendererReEntrantManager.cs @@ -0,0 +1,120 @@ +using System; +#if NET35 +using System.Web; +#else +using System.Threading; +#endif + +namespace NLog.Web.Internal +{ + /// + /// 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 + /// + internal class RendererReEntrantManager : IDisposable + { +#if NET35 + private static readonly object ReEntrantLock = new object(); + + internal RendererReEntrantManager(HttpContextBase context) + { + httpContext = context; + } + + private readonly HttpContextBase httpContext; + + private bool ReadLock() + { + return httpContext?.Items?.Contains(ReEntrantLock) == true; + } + + private void Lock() + { + httpContext.Items[ReEntrantLock] = bool.TrueString; + } + + private void Unlock() + { + httpContext?.Items?.Remove(ReEntrantLock); + } +#else + // Manage access to the session re-entrancy, at least above .NET 3.5 + private static readonly AsyncLocal ReEntrantLock = new AsyncLocal(); + + internal RendererReEntrantManager() + { + + } + + private bool ReadLock() + { + return ReEntrantLock.Value; + } + + private void Lock() + { + ReEntrantLock.Value = true; + } + + private void Unlock() + { + ReEntrantLock.Value = false; + } +#endif + + // Need to track if we were successful in the entry + // If we were not, we should not unlock in the dispose code + private bool entrySuccess; + + + internal bool TryGetLock() + { + // If already locked, return false + if (ReadLock()) + { + return false; + } + // Get the lock + Lock(); + // Mark that we locked it, not another instance locked it + entrySuccess = true; + // Return to the caller that we locked it + return true; + } + + private void DisposalImpl() + { + // Only unlock if we were the ones who locked it + if (entrySuccess) + { + Unlock(); + } + } + + // Dispose methods + // Below code generated by Visual Studio 2022 and Resharper + private bool disposedValue; + + protected virtual void Dispose(bool disposing) + { + if (!disposedValue) + { + if (disposing) + { + DisposalImpl(); + } + disposedValue = true; + } + } + + public void Dispose() + { + // Do not change this code. Put cleanup code in 'Dispose(bool disposing)' method + Dispose(disposing: true); + GC.SuppressFinalize(this); + } + } +} diff --git a/src/Shared/LayoutRenderers/AspNetSessionValueLayoutRenderer.cs b/src/Shared/LayoutRenderers/AspNetSessionValueLayoutRenderer.cs index 83a731f8..3fa026af 100644 --- a/src/Shared/LayoutRenderers/AspNetSessionValueLayoutRenderer.cs +++ b/src/Shared/LayoutRenderers/AspNetSessionValueLayoutRenderer.cs @@ -1,6 +1,5 @@ using System.Globalization; using System.Text; -using System.Threading; using NLog.Config; using NLog.LayoutRenderers; using NLog.Web.Internal; @@ -80,13 +79,6 @@ public class AspNetSessionValueLayoutRenderer : AspNetLayoutRendererBase public SessionValueType ValueType { get; set; } = SessionValueType.String; #endif -#if !NET35 - // Manage access to the session re-entrancy, at least above .NET 3.5 - private static readonly AsyncLocal IsReEntrant = new AsyncLocal(); -#else - // NET 35 does not have AsyncLocal or even older ThreadLocal - private static readonly object IsReEntrant = new object(); -#endif /// protected override void DoAppend(StringBuilder builder, LogEventInfo logEvent) { @@ -107,51 +99,30 @@ protected override void DoAppend(StringBuilder builder, LogEventInfo logEvent) { return; } -#if !NET35 - // If we are already in this layout render in the same path, we should stop the recursion - if (IsReEntrant.Value) - { - InternalLogger.Error($"Reentrant log event detected. Logging when inside the scope of another log event can cause a StackOverflowException. LogEventInfo.Message:{logEvent.Message}"); - return; - } - // Mark that we have entered the session - IsReEntrant.Value = true; -#else - if (context.Items == null) - { - return; - } - // If we are already in this layout render in the same path, we should stop the recursion - if (context.Items.Contains(IsReEntrant)) - { - InternalLogger.Error($"Reentrant log event detected. Logging when inside the scope of another log event can cause a StackOverflowException. LogEventInfo.Message:{logEvent.Message}"); - return; - } - // Mark that we have entered the session - context.Items[IsReEntrant] = bool.TrueString; + using (var reEntry = new RendererReEntrantManager( +#if NET35 + context #endif - // Perform the PropertyReader.GetValue() in a try/finally clause since we want to set the IsReEntrant to false even if there is an Exception - object value; - try - { - value = PropertyReader.GetValue(item, contextSession, GetSessionValue, EvaluateAsNestedProperties); - } - finally + )) { -#if !NET35 - IsReEntrant.Value = false; -#else - context.Items.Remove(IsReEntrant); -#endif - } + if (!reEntry.TryGetLock()) + { + InternalLogger.Error( + $"Reentrant log event detected. Logging when inside the scope of another log event can cause a StackOverflowException. LogEventInfo.Message:{logEvent.Message}"); + return; + } - if (value != null) - { - var formatProvider = GetFormatProvider(logEvent, Culture); - builder.AppendFormattedValue(value, Format, formatProvider, ValueFormatter); + var value = PropertyReader.GetValue(item, contextSession, GetSessionValue, EvaluateAsNestedProperties); + + if (value != null) + { + var formatProvider = GetFormatProvider(logEvent, Culture); + builder.AppendFormattedValue(value, Format, formatProvider, ValueFormatter); + } } } + #if ASP_NET_CORE private object GetSessionValue(ISession session, string key) { @@ -169,5 +140,6 @@ private object GetSessionValue(HttpSessionStateBase session, string key) return session.Count == 0 ? null : session[key]; } #endif + } } \ No newline at end of file From 68842b49d994b7887ffd15e526441d93eef81326 Mon Sep 17 00:00:00 2001 From: Burak Akgerman Date: Thu, 11 Aug 2022 12:07:34 -0400 Subject: [PATCH 05/18] Rename a method in ReEntrantManager to make more sense --- src/Shared/Internal/RendererReEntrantManager.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/Shared/Internal/RendererReEntrantManager.cs b/src/Shared/Internal/RendererReEntrantManager.cs index e9deb087..99dc8856 100644 --- a/src/Shared/Internal/RendererReEntrantManager.cs +++ b/src/Shared/Internal/RendererReEntrantManager.cs @@ -26,7 +26,7 @@ internal RendererReEntrantManager(HttpContextBase context) private readonly HttpContextBase httpContext; - private bool ReadLock() + private bool IsLocked() { return httpContext?.Items?.Contains(ReEntrantLock) == true; } @@ -49,7 +49,7 @@ internal RendererReEntrantManager() } - private bool ReadLock() + private bool IsLocked() { return ReEntrantLock.Value; } @@ -73,7 +73,7 @@ private void Unlock() internal bool TryGetLock() { // If already locked, return false - if (ReadLock()) + if (IsLocked()) { return false; } From b0c38e0901d5217cb68c8bcc4ed87855462262be Mon Sep 17 00:00:00 2001 From: Burak Akgerman Date: Thu, 11 Aug 2022 12:10:26 -0400 Subject: [PATCH 06/18] rename a variable to make more sense --- src/Shared/Internal/RendererReEntrantManager.cs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/Shared/Internal/RendererReEntrantManager.cs b/src/Shared/Internal/RendererReEntrantManager.cs index 99dc8856..1ea4098e 100644 --- a/src/Shared/Internal/RendererReEntrantManager.cs +++ b/src/Shared/Internal/RendererReEntrantManager.cs @@ -65,9 +65,9 @@ private void Unlock() } #endif - // Need to track if we were successful in the entry + // Need to track if we were successful in the lock // If we were not, we should not unlock in the dispose code - private bool entrySuccess; + private bool obtainedLock; internal bool TryGetLock() @@ -80,7 +80,7 @@ internal bool TryGetLock() // Get the lock Lock(); // Mark that we locked it, not another instance locked it - entrySuccess = true; + obtainedLock = true; // Return to the caller that we locked it return true; } @@ -88,7 +88,7 @@ internal bool TryGetLock() private void DisposalImpl() { // Only unlock if we were the ones who locked it - if (entrySuccess) + if (obtainedLock) { Unlock(); } From 6b5577f51bfe7fefd70a3d646739724b3f842a5a Mon Sep 17 00:00:00 2001 From: Burak Akgerman Date: Thu, 11 Aug 2022 12:15:27 -0400 Subject: [PATCH 07/18] review changes --- .../Internal/RendererReEntrantManager.cs | 40 +++++-------------- 1 file changed, 11 insertions(+), 29 deletions(-) diff --git a/src/Shared/Internal/RendererReEntrantManager.cs b/src/Shared/Internal/RendererReEntrantManager.cs index 1ea4098e..3e37d202 100644 --- a/src/Shared/Internal/RendererReEntrantManager.cs +++ b/src/Shared/Internal/RendererReEntrantManager.cs @@ -14,34 +14,34 @@ namespace NLog.Web.Internal /// Since NET 35 does not support AsyncLocal or even ThreadLocal /// a different technique must be used for that platform /// - internal class RendererReEntrantManager : IDisposable + internal struct RendererReEntrantManager : IDisposable { #if NET35 private static readonly object ReEntrantLock = new object(); internal RendererReEntrantManager(HttpContextBase context) { - httpContext = context; + _httpContext = context; + _obtainedLock = false; } - private readonly HttpContextBase httpContext; + private readonly HttpContextBase _httpContext; private bool IsLocked() { - return httpContext?.Items?.Contains(ReEntrantLock) == true; + return _httpContext?.Items?.Contains(ReEntrantLock) == true; } private void Lock() { - httpContext.Items[ReEntrantLock] = bool.TrueString; + _httpContext.Items[ReEntrantLock] = bool.TrueString; } private void Unlock() { - httpContext?.Items?.Remove(ReEntrantLock); + _httpContext?.Items?.Remove(ReEntrantLock); } #else - // Manage access to the session re-entrancy, at least above .NET 3.5 private static readonly AsyncLocal ReEntrantLock = new AsyncLocal(); internal RendererReEntrantManager() @@ -67,7 +67,7 @@ private void Unlock() // Need to track if we were successful in the lock // If we were not, we should not unlock in the dispose code - private bool obtainedLock; + private bool _obtainedLock; internal bool TryGetLock() @@ -80,7 +80,7 @@ internal bool TryGetLock() // Get the lock Lock(); // Mark that we locked it, not another instance locked it - obtainedLock = true; + _obtainedLock = true; // Return to the caller that we locked it return true; } @@ -88,33 +88,15 @@ internal bool TryGetLock() private void DisposalImpl() { // Only unlock if we were the ones who locked it - if (obtainedLock) + if (_obtainedLock) { Unlock(); } } - // Dispose methods - // Below code generated by Visual Studio 2022 and Resharper - private bool disposedValue; - - protected virtual void Dispose(bool disposing) - { - if (!disposedValue) - { - if (disposing) - { - DisposalImpl(); - } - disposedValue = true; - } - } - public void Dispose() { - // Do not change this code. Put cleanup code in 'Dispose(bool disposing)' method - Dispose(disposing: true); - GC.SuppressFinalize(this); + DisposalImpl(); } } } From 13d6a92510e0f5cd08c15b440ab91db7ccb51d91 Mon Sep 17 00:00:00 2001 From: Burak Akgerman Date: Thu, 11 Aug 2022 12:19:33 -0400 Subject: [PATCH 08/18] committed too soon, now fixed --- .../Internal/RendererReEntrantManager.cs | 20 +++++++++---------- .../AspNetSessionValueLayoutRenderer.cs | 6 +----- 2 files changed, 10 insertions(+), 16 deletions(-) diff --git a/src/Shared/Internal/RendererReEntrantManager.cs b/src/Shared/Internal/RendererReEntrantManager.cs index 3e37d202..3dfab8bc 100644 --- a/src/Shared/Internal/RendererReEntrantManager.cs +++ b/src/Shared/Internal/RendererReEntrantManager.cs @@ -1,9 +1,12 @@ using System; -#if NET35 -using System.Web; -#else +#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 { @@ -16,9 +19,6 @@ namespace NLog.Web.Internal /// internal struct RendererReEntrantManager : IDisposable { -#if NET35 - private static readonly object ReEntrantLock = new object(); - internal RendererReEntrantManager(HttpContextBase context) { _httpContext = context; @@ -27,6 +27,9 @@ internal RendererReEntrantManager(HttpContextBase context) private readonly HttpContextBase _httpContext; +#if NET35 + private static readonly object ReEntrantLock = new object(); + private bool IsLocked() { return _httpContext?.Items?.Contains(ReEntrantLock) == true; @@ -44,11 +47,6 @@ private void Unlock() #else private static readonly AsyncLocal ReEntrantLock = new AsyncLocal(); - internal RendererReEntrantManager() - { - - } - private bool IsLocked() { return ReEntrantLock.Value; diff --git a/src/Shared/LayoutRenderers/AspNetSessionValueLayoutRenderer.cs b/src/Shared/LayoutRenderers/AspNetSessionValueLayoutRenderer.cs index 3fa026af..c35caa29 100644 --- a/src/Shared/LayoutRenderers/AspNetSessionValueLayoutRenderer.cs +++ b/src/Shared/LayoutRenderers/AspNetSessionValueLayoutRenderer.cs @@ -100,11 +100,7 @@ protected override void DoAppend(StringBuilder builder, LogEventInfo logEvent) return; } - using (var reEntry = new RendererReEntrantManager( -#if NET35 - context -#endif - )) + using (var reEntry = new RendererReEntrantManager(context)) { if (!reEntry.TryGetLock()) { From eca59da909340ca33fe297a2d57f38126b73e5b7 Mon Sep 17 00:00:00 2001 From: Burak Akgerman Date: Thu, 11 Aug 2022 12:58:53 -0400 Subject: [PATCH 09/18] Attmpt to obtain the lock in the ctor of ReEntrantManager --- src/Shared/Internal/RendererReEntrantManager.cs | 9 +++++---- .../LayoutRenderers/AspNetSessionValueLayoutRenderer.cs | 2 +- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/src/Shared/Internal/RendererReEntrantManager.cs b/src/Shared/Internal/RendererReEntrantManager.cs index 3dfab8bc..64846c0b 100644 --- a/src/Shared/Internal/RendererReEntrantManager.cs +++ b/src/Shared/Internal/RendererReEntrantManager.cs @@ -23,10 +23,13 @@ internal RendererReEntrantManager(HttpContextBase context) { _httpContext = context; _obtainedLock = false; + TryGetLock(); } private readonly HttpContextBase _httpContext; + internal bool IsLockAcquired => _obtainedLock; + #if NET35 private static readonly object ReEntrantLock = new object(); @@ -68,19 +71,17 @@ private void Unlock() private bool _obtainedLock; - internal bool TryGetLock() + private void TryGetLock() { // If already locked, return false if (IsLocked()) { - return false; + return; } // Get the lock Lock(); // Mark that we locked it, not another instance locked it _obtainedLock = true; - // Return to the caller that we locked it - return true; } private void DisposalImpl() diff --git a/src/Shared/LayoutRenderers/AspNetSessionValueLayoutRenderer.cs b/src/Shared/LayoutRenderers/AspNetSessionValueLayoutRenderer.cs index c35caa29..77651dbc 100644 --- a/src/Shared/LayoutRenderers/AspNetSessionValueLayoutRenderer.cs +++ b/src/Shared/LayoutRenderers/AspNetSessionValueLayoutRenderer.cs @@ -102,7 +102,7 @@ protected override void DoAppend(StringBuilder builder, LogEventInfo logEvent) using (var reEntry = new RendererReEntrantManager(context)) { - if (!reEntry.TryGetLock()) + if (!reEntry.IsLockAcquired) { InternalLogger.Error( $"Reentrant log event detected. Logging when inside the scope of another log event can cause a StackOverflowException. LogEventInfo.Message:{logEvent.Message}"); From c3272c43b6fa2fa05dcba8092f705ec2a3f38f3d Mon Sep 17 00:00:00 2001 From: Burak Akgerman Date: Thu, 11 Aug 2022 13:23:16 -0400 Subject: [PATCH 10/18] ctor and InternalLogger changes --- .../Internal/RendererReEntrantManager.cs | 42 ++++++++++--------- .../AspNetSessionValueLayoutRenderer.cs | 4 +- 2 files changed, 24 insertions(+), 22 deletions(-) diff --git a/src/Shared/Internal/RendererReEntrantManager.cs b/src/Shared/Internal/RendererReEntrantManager.cs index 64846c0b..4df07791 100644 --- a/src/Shared/Internal/RendererReEntrantManager.cs +++ b/src/Shared/Internal/RendererReEntrantManager.cs @@ -17,19 +17,39 @@ namespace NLog.Web.Internal /// Since NET 35 does not support AsyncLocal or even ThreadLocal /// a different technique must be used for that platform /// - internal struct RendererReEntrantManager : IDisposable + internal readonly struct RendererReEntrantManager : IDisposable { internal RendererReEntrantManager(HttpContextBase context) { _httpContext = context; + + // The line below is required, because this is a struct, otherwise we get CS0188 compiler error + // which is 'The 'this' object cannot be used before all of its fields have been assigned.' _obtainedLock = false; - TryGetLock(); + + _obtainedLock = 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 + private readonly bool _obtainedLock; + internal bool IsLockAcquired => _obtainedLock; + private bool TryGetLock() + { + // If already locked, return false + if (IsLocked()) + { + return false; + } + // Get the lock + Lock(); + return true; + } + #if NET35 private static readonly object ReEntrantLock = new object(); @@ -66,24 +86,6 @@ private void Unlock() } #endif - // Need to track if we were successful in the lock - // If we were not, we should not unlock in the dispose code - private bool _obtainedLock; - - - private void TryGetLock() - { - // If already locked, return false - if (IsLocked()) - { - return; - } - // Get the lock - Lock(); - // Mark that we locked it, not another instance locked it - _obtainedLock = true; - } - private void DisposalImpl() { // Only unlock if we were the ones who locked it diff --git a/src/Shared/LayoutRenderers/AspNetSessionValueLayoutRenderer.cs b/src/Shared/LayoutRenderers/AspNetSessionValueLayoutRenderer.cs index 77651dbc..f6d68881 100644 --- a/src/Shared/LayoutRenderers/AspNetSessionValueLayoutRenderer.cs +++ b/src/Shared/LayoutRenderers/AspNetSessionValueLayoutRenderer.cs @@ -104,8 +104,8 @@ protected override void DoAppend(StringBuilder builder, LogEventInfo logEvent) { if (!reEntry.IsLockAcquired) { - InternalLogger.Error( - $"Reentrant log event detected. Logging when inside the scope of another log event can cause a StackOverflowException. LogEventInfo.Message:{logEvent.Message}"); + InternalLogger.Debug( + $"Reentrant log event detected. Logging when inside the scope of another log event can cause a StackOverflowException."); return; } From 990f8d1eb666de45290c63b00a55ea0235c11fb3 Mon Sep 17 00:00:00 2001 From: Burak Akgerman Date: Thu, 11 Aug 2022 13:33:05 -0400 Subject: [PATCH 11/18] review changes --- .../LayoutRenderers/AspNetSessionValueLayoutRenderer.cs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/Shared/LayoutRenderers/AspNetSessionValueLayoutRenderer.cs b/src/Shared/LayoutRenderers/AspNetSessionValueLayoutRenderer.cs index f6d68881..43045167 100644 --- a/src/Shared/LayoutRenderers/AspNetSessionValueLayoutRenderer.cs +++ b/src/Shared/LayoutRenderers/AspNetSessionValueLayoutRenderer.cs @@ -100,12 +100,15 @@ protected override void DoAppend(StringBuilder builder, LogEventInfo logEvent) return; } + // 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 reEntry = new RendererReEntrantManager(context)) { if (!reEntry.IsLockAcquired) { InternalLogger.Debug( - $"Reentrant log event detected. Logging when inside the scope of another log event can cause a StackOverflowException."); + "Reentrant log event detected. Logging when inside the scope of another log event can cause a StackOverflowException."); return; } From ca961c9c89765fb63cc4ba9640546afc681657f1 Mon Sep 17 00:00:00 2001 From: Burak Akgerman Date: Thu, 11 Aug 2022 14:25:04 -0400 Subject: [PATCH 12/18] rename class as requested --- src/Shared/Internal/RendererReEntrantManager.cs | 4 ++-- .../LayoutRenderers/AspNetSessionValueLayoutRenderer.cs | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/Shared/Internal/RendererReEntrantManager.cs b/src/Shared/Internal/RendererReEntrantManager.cs index 4df07791..6ad0b68a 100644 --- a/src/Shared/Internal/RendererReEntrantManager.cs +++ b/src/Shared/Internal/RendererReEntrantManager.cs @@ -17,9 +17,9 @@ namespace NLog.Web.Internal /// Since NET 35 does not support AsyncLocal or even ThreadLocal /// a different technique must be used for that platform /// - internal readonly struct RendererReEntrantManager : IDisposable + internal readonly struct ReEntrantScopeLock : IDisposable { - internal RendererReEntrantManager(HttpContextBase context) + internal ReEntrantScopeLock(HttpContextBase context) { _httpContext = context; diff --git a/src/Shared/LayoutRenderers/AspNetSessionValueLayoutRenderer.cs b/src/Shared/LayoutRenderers/AspNetSessionValueLayoutRenderer.cs index 43045167..dbde34e9 100644 --- a/src/Shared/LayoutRenderers/AspNetSessionValueLayoutRenderer.cs +++ b/src/Shared/LayoutRenderers/AspNetSessionValueLayoutRenderer.cs @@ -103,7 +103,7 @@ protected override void DoAppend(StringBuilder builder, LogEventInfo logEvent) // 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 reEntry = new RendererReEntrantManager(context)) + using (var reEntry = new ReEntrantScopeLock(context)) { if (!reEntry.IsLockAcquired) { From 89d758807a35fae4ee9d2e8e0abfa0f49492c683 Mon Sep 17 00:00:00 2001 From: Burak Akgerman Date: Thu, 11 Aug 2022 14:34:34 -0400 Subject: [PATCH 13/18] Renamed --- .../{RendererReEntrantManager.cs => ReEntrantScopeLock.cs} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename src/Shared/Internal/{RendererReEntrantManager.cs => ReEntrantScopeLock.cs} (100%) diff --git a/src/Shared/Internal/RendererReEntrantManager.cs b/src/Shared/Internal/ReEntrantScopeLock.cs similarity index 100% rename from src/Shared/Internal/RendererReEntrantManager.cs rename to src/Shared/Internal/ReEntrantScopeLock.cs From 331d6ab876f3af9585a47b2b138eacbc61a5541b Mon Sep 17 00:00:00 2001 From: Burak Akgerman Date: Thu, 11 Aug 2022 15:00:32 -0400 Subject: [PATCH 14/18] Simplify Disposal by eliminate separate DisposalImpl method --- src/Shared/Internal/ReEntrantScopeLock.cs | 23 +++++++++-------------- 1 file changed, 9 insertions(+), 14 deletions(-) diff --git a/src/Shared/Internal/ReEntrantScopeLock.cs b/src/Shared/Internal/ReEntrantScopeLock.cs index 6ad0b68a..6a0f0b75 100644 --- a/src/Shared/Internal/ReEntrantScopeLock.cs +++ b/src/Shared/Internal/ReEntrantScopeLock.cs @@ -50,6 +50,15 @@ private bool TryGetLock() return true; } + public void Dispose() + { + // Only unlock if we were the ones who locked it + if (_obtainedLock) + { + Unlock(); + } + } + #if NET35 private static readonly object ReEntrantLock = new object(); @@ -85,19 +94,5 @@ private void Unlock() ReEntrantLock.Value = false; } #endif - - private void DisposalImpl() - { - // Only unlock if we were the ones who locked it - if (_obtainedLock) - { - Unlock(); - } - } - - public void Dispose() - { - DisposalImpl(); - } } } From f02ee7d51f3b19ba211cef3990458fdc818c8bd7 Mon Sep 17 00:00:00 2001 From: Burak Akgerman Date: Fri, 12 Aug 2022 10:04:48 -0400 Subject: [PATCH 15/18] Implement the requested change --- src/Shared/Internal/ReEntrantScopeLock.cs | 5 ----- 1 file changed, 5 deletions(-) diff --git a/src/Shared/Internal/ReEntrantScopeLock.cs b/src/Shared/Internal/ReEntrantScopeLock.cs index 6a0f0b75..12fc18c7 100644 --- a/src/Shared/Internal/ReEntrantScopeLock.cs +++ b/src/Shared/Internal/ReEntrantScopeLock.cs @@ -22,11 +22,6 @@ namespace NLog.Web.Internal internal ReEntrantScopeLock(HttpContextBase context) { _httpContext = context; - - // The line below is required, because this is a struct, otherwise we get CS0188 compiler error - // which is 'The 'this' object cannot be used before all of its fields have been assigned.' - _obtainedLock = false; - _obtainedLock = TryGetLock(); } From 951a9c88eeeadc052199cd1a05eac2174bb93adb Mon Sep 17 00:00:00 2001 From: Burak Akgerman Date: Fri, 12 Aug 2022 10:30:12 -0400 Subject: [PATCH 16/18] Fxied to use static TryGetLock() method --- src/Shared/Internal/ReEntrantScopeLock.cs | 43 +++++++++++++---------- 1 file changed, 25 insertions(+), 18 deletions(-) diff --git a/src/Shared/Internal/ReEntrantScopeLock.cs b/src/Shared/Internal/ReEntrantScopeLock.cs index 12fc18c7..d030abf8 100644 --- a/src/Shared/Internal/ReEntrantScopeLock.cs +++ b/src/Shared/Internal/ReEntrantScopeLock.cs @@ -22,69 +22,76 @@ namespace NLog.Web.Internal internal ReEntrantScopeLock(HttpContextBase context) { _httpContext = context; - _obtainedLock = TryGetLock(); + IsLockAcquired = TryGetLock(context); } 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 - private readonly bool _obtainedLock; + internal bool IsLockAcquired { get; } - internal bool IsLockAcquired => _obtainedLock; - - private bool TryGetLock() + private static bool TryGetLock(HttpContextBase context) { + // If context is null leave + if (context == null) + { + return false; + } + // If already locked, return false - if (IsLocked()) + if (IsLocked(context)) { return false; } + // Get the lock - Lock(); + Lock(context); + + // Indicate the lock was successfully acquired return true; } public void Dispose() { // Only unlock if we were the ones who locked it - if (_obtainedLock) + if (IsLockAcquired) { - Unlock(); + Unlock(_httpContext); } } #if NET35 private static readonly object ReEntrantLock = new object(); - private bool IsLocked() + private static bool IsLocked(HttpContextBase context) { - return _httpContext?.Items?.Contains(ReEntrantLock) == true; + return context.Items?.Contains(ReEntrantLock) == true; } - private void Lock() + private static void Lock(HttpContextBase context) { - _httpContext.Items[ReEntrantLock] = bool.TrueString; + context.Items[ReEntrantLock] = bool.TrueString; } - private void Unlock() + private static void Unlock(HttpContextBase context) { - _httpContext?.Items?.Remove(ReEntrantLock); + context.Items?.Remove(ReEntrantLock); } #else private static readonly AsyncLocal ReEntrantLock = new AsyncLocal(); - private bool IsLocked() + private static bool IsLocked(HttpContextBase context) { return ReEntrantLock.Value; } - private void Lock() + private static void Lock(HttpContextBase context) { ReEntrantLock.Value = true; } - private void Unlock() + private static void Unlock(HttpContextBase context) { ReEntrantLock.Value = false; } From 3b5f19664d9b876a1769226a9116b3ba2782998f Mon Sep 17 00:00:00 2001 From: Burak Akgerman Date: Fri, 12 Aug 2022 10:39:03 -0400 Subject: [PATCH 17/18] Split the code to eliminate compiler warnings --- src/Shared/Internal/ReEntrantScopeLock.cs | 42 +++++++++++++++++++---- 1 file changed, 36 insertions(+), 6 deletions(-) diff --git a/src/Shared/Internal/ReEntrantScopeLock.cs b/src/Shared/Internal/ReEntrantScopeLock.cs index d030abf8..3ba1db13 100644 --- a/src/Shared/Internal/ReEntrantScopeLock.cs +++ b/src/Shared/Internal/ReEntrantScopeLock.cs @@ -31,6 +31,9 @@ internal ReEntrantScopeLock(HttpContextBase context) // 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 @@ -61,9 +64,6 @@ public void Dispose() } } -#if NET35 - private static readonly object ReEntrantLock = new object(); - private static bool IsLocked(HttpContextBase context) { return context.Items?.Contains(ReEntrantLock) == true; @@ -81,17 +81,47 @@ private static void Unlock(HttpContextBase context) #else private static readonly AsyncLocal ReEntrantLock = new AsyncLocal(); - private static bool IsLocked(HttpContextBase context) + private static bool TryGetLock(HttpContextBase context) + { + // If context is null leave + if (context == null) + { + return false; + } + + // If already locked, return false + if (IsLocked()) + { + return false; + } + + // Get the lock + Lock(); + + // Indicate the lock was successfully acquired + return true; + } + + public void Dispose() + { + // Only unlock if we were the ones who locked it + if (IsLockAcquired) + { + Unlock(); + } + } + + private static bool IsLocked() { return ReEntrantLock.Value; } - private static void Lock(HttpContextBase context) + private static void Lock() { ReEntrantLock.Value = true; } - private static void Unlock(HttpContextBase context) + private static void Unlock() { ReEntrantLock.Value = false; } From 7a52795c022176e6327f3f01fb409b030eeb4e18 Mon Sep 17 00:00:00 2001 From: Burak Akgerman Date: Fri, 12 Aug 2022 11:45:27 -0400 Subject: [PATCH 18/18] reduce number of private methods --- src/Shared/Internal/ReEntrantScopeLock.cs | 42 ++++------------------- 1 file changed, 6 insertions(+), 36 deletions(-) diff --git a/src/Shared/Internal/ReEntrantScopeLock.cs b/src/Shared/Internal/ReEntrantScopeLock.cs index 3ba1db13..67bf30c7 100644 --- a/src/Shared/Internal/ReEntrantScopeLock.cs +++ b/src/Shared/Internal/ReEntrantScopeLock.cs @@ -43,13 +43,13 @@ private static bool TryGetLock(HttpContextBase context) } // If already locked, return false - if (IsLocked(context)) + if (context.Items?.Contains(ReEntrantLock) == true) { return false; } // Get the lock - Lock(context); + context.Items[ReEntrantLock] = bool.TrueString; // Indicate the lock was successfully acquired return true; @@ -60,24 +60,9 @@ public void Dispose() // Only unlock if we were the ones who locked it if (IsLockAcquired) { - Unlock(_httpContext); + _httpContext.Items?.Remove(ReEntrantLock); } } - - private static bool IsLocked(HttpContextBase context) - { - return context.Items?.Contains(ReEntrantLock) == true; - } - - private static void Lock(HttpContextBase context) - { - context.Items[ReEntrantLock] = bool.TrueString; - } - - private static void Unlock(HttpContextBase context) - { - context.Items?.Remove(ReEntrantLock); - } #else private static readonly AsyncLocal ReEntrantLock = new AsyncLocal(); @@ -90,13 +75,13 @@ private static bool TryGetLock(HttpContextBase context) } // If already locked, return false - if (IsLocked()) + if (ReEntrantLock.Value) { return false; } // Get the lock - Lock(); + ReEntrantLock.Value = true; // Indicate the lock was successfully acquired return true; @@ -107,24 +92,9 @@ public void Dispose() // Only unlock if we were the ones who locked it if (IsLockAcquired) { - Unlock(); + ReEntrantLock.Value = false; } } - - private static bool IsLocked() - { - return ReEntrantLock.Value; - } - - private static void Lock() - { - ReEntrantLock.Value = true; - } - - private static void Unlock() - { - ReEntrantLock.Value = false; - } #endif } }