Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AspNetSessionValueLayoutRenderer - Skip ReEntrantScopeLock for classic NLog.Web #853

Merged
merged 1 commit into from
Aug 12, 2022
Merged

AspNetSessionValueLayoutRenderer - Skip ReEntrantScopeLock for classic NLog.Web #853

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
72 changes: 11 additions & 61 deletions src/Shared/Internal/ReEntrantScopeLock.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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)
{
Expand All @@ -95,6 +43,8 @@ public void Dispose()
ReEntrantLock.Value = false;
}
}
#endif
}
}


#endif
27 changes: 22 additions & 5 deletions src/Shared/LayoutRenderers/AspNetSessionIdLayoutRenderer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
using System.Text;
using NLog.Common;
using NLog.Config;
using NLog.LayoutRenderers;
using NLog.Web.Internal;
Expand All @@ -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
}
}
}
}
64 changes: 41 additions & 23 deletions src/Shared/LayoutRenderers/AspNetSessionValueLayoutRenderer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -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)
{
Expand All @@ -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);
Expand All @@ -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];
}
Expand Down