-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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.
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
Callsite fix for async methods #1138
Changes from 7 commits
23f1cc2
0edc413
774bf51
0ebe032
44f8728
ab5a1d5
b9ada9c
76e2225
059bf34
78d3cc1
ab025b0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -114,21 +114,54 @@ private static int FindCallingMethodOnStackTrace([NotNull] StackTrace stackTrace | |
var stackFrames = stackTrace.GetFrames(); | ||
if (stackFrames == null) | ||
return 0; | ||
var intermediate = stackFrames.Select((f, i) => new {index = i, frame = f}); | ||
var intermediate = stackFrames.Select((f, i) => new StackFrameWithIndex(i, f)); | ||
//find until logger type | ||
intermediate = intermediate.SkipWhile(p => !IsLoggerType(p.frame, loggerType)); | ||
//skip to next | ||
intermediate = intermediate.Skip(1); | ||
//skip while in "skip" assemlbl | ||
intermediate = intermediate.SkipWhile(p => SkipAssembly(p.frame)); | ||
intermediate = intermediate.SkipWhile(p => !IsLoggerType(p.StackFrame, loggerType)); | ||
|
||
intermediate = FilterBySkipAssembly(intermediate); | ||
return GetIndexOfNonAsync(intermediate); | ||
} | ||
|
||
private static int GetIndexOfNonAsync(IEnumerable<StackFrameWithIndex> intermediate) | ||
{ | ||
var last = intermediate.FirstOrDefault(); | ||
|
||
if (last != null) | ||
{ | ||
return last.index; | ||
#if ASYNC_SUPPORTED | ||
|
||
//movenext and then AsyncTaskMethodBuilder (method start)? this is a generated MoveNext by async. | ||
if (last.StackFrame.GetMethod().Name == "MoveNext") | ||
{ | ||
var next = intermediate.Skip(1).FirstOrDefault(); | ||
if (next != null) | ||
{ | ||
var declaringType = next.StackFrame.GetMethod().DeclaringType; | ||
if (declaringType != null && declaringType.FullName == "System.Runtime.CompilerServices.AsyncTaskMethodBuilder") | ||
{ | ||
|
||
//async, search futher | ||
|
||
intermediate = FilterBySkipAssembly(intermediate); | ||
return GetIndexOfNonAsync(intermediate); | ||
} | ||
} | ||
} | ||
#endif | ||
|
||
return last.StackFrameIndex; | ||
} | ||
return 0; | ||
} | ||
|
||
private static IEnumerable<StackFrameWithIndex> FilterBySkipAssembly(IEnumerable<StackFrameWithIndex> intermediate) | ||
{ | ||
|
||
//skip to next | ||
intermediate = intermediate.Skip(1); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure this line belongs inside this method. The name of the method does not say that it's skipping the next element. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point. Will move it. |
||
//skip while in "skip" assemlbl | ||
intermediate = intermediate.SkipWhile(p => SkipAssembly(p.StackFrame)); | ||
return intermediate; | ||
} | ||
|
||
/// <summary> | ||
|
@@ -249,5 +282,32 @@ private static FilterResult GetFilterResult(IList<Filter> filterChain, LogEventI | |
return FilterResult.Ignore; | ||
} | ||
} | ||
|
||
/// <summary> | ||
/// Stackframe with correspending index on the stracktrace | ||
/// </summary> | ||
private class StackFrameWithIndex | ||
{ | ||
/// <summary> | ||
/// Index of <see cref="StackFrame"/> on the stack. | ||
/// </summary> | ||
public int StackFrameIndex { get; private set; } | ||
|
||
/// <summary> | ||
/// A stackframe | ||
/// </summary> | ||
public StackFrame StackFrame { get; private set; } | ||
|
||
/// <summary> | ||
/// New item | ||
/// </summary> | ||
/// <param name="stackFrameIndex">Index of <paramref name="stackFrame"/> on the stack.</param> | ||
/// <param name="stackFrame">A stackframe</param> | ||
public StackFrameWithIndex(int stackFrameIndex, StackFrame stackFrame) | ||
{ | ||
StackFrameIndex = stackFrameIndex; | ||
StackFrame = stackFrame; | ||
} | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -31,6 +31,9 @@ | |
// THE POSSIBILITY OF SUCH DAMAGE. | ||
// | ||
|
||
using System.IO; | ||
using System.Threading.Tasks; | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Must these There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would be more consistent yes. Resharper did this for me, but will fix it (cannot configure this in Resharper AFAIK) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I just noticed Visual Studio's "Quick Actions" adds the |
||
namespace NLog.UnitTests.LayoutRenderers | ||
{ | ||
using System; | ||
|
@@ -191,7 +194,7 @@ public void ClassNameWithPaddingTestPadLeftAlignLeftTest() | |
MethodBase currentMethod = MethodBase.GetCurrentMethod(); | ||
AssertDebugLastMessage("debug", currentMethod.DeclaringType.FullName.Substring(0, 3) + " msg"); | ||
} | ||
|
||
[Fact] | ||
public void ClassNameWithPaddingTestPadLeftAlignRightTest() | ||
{ | ||
|
@@ -548,7 +551,130 @@ public void When_WrappedInCompsition_Ignore_Wrapper_Methods_In_Callstack() | |
|
||
} | ||
|
||
#if ASYNC_SUPPORTED | ||
[Fact] | ||
public void Show_correct_method_with_async() | ||
{ | ||
|
||
//namespace en name of current method | ||
const string currentMethodFullName = "NLog.UnitTests.LayoutRenderers.CallSiteTests.AsyncMethod"; | ||
|
||
LogManager.Configuration = CreateConfigurationFromString(@" | ||
<nlog> | ||
<targets><target name='debug' type='Debug' layout='${callsite}|${message}' /></targets> | ||
<rules> | ||
<logger name='*' levels='Warn' writeTo='debug' /> | ||
</rules> | ||
</nlog>"); | ||
|
||
AsyncMethod().Wait(); | ||
AssertDebugLastMessage("debug", string.Format("{0}|direct", currentMethodFullName)); | ||
|
||
} | ||
|
||
private async Task AsyncMethod() | ||
{ | ||
var logger = LogManager.GetCurrentClassLogger(); | ||
logger.Warn("direct"); | ||
var reader = new StreamReader(new MemoryStream(new byte[0])); | ||
await reader.ReadLineAsync(); | ||
} | ||
|
||
[Fact] | ||
public void Show_correct_method_with_async2() | ||
{ | ||
|
||
//namespace en name of current method | ||
const string currentMethodFullName = "NLog.UnitTests.LayoutRenderers.CallSiteTests.AsyncMethod2b"; | ||
|
||
LogManager.Configuration = CreateConfigurationFromString(@" | ||
<nlog> | ||
<targets><target name='debug' type='Debug' layout='${callsite}|${message}' /></targets> | ||
<rules> | ||
<logger name='*' levels='Warn' writeTo='debug' /> | ||
</rules> | ||
</nlog>"); | ||
|
||
AsyncMethod2a().Wait(); | ||
AssertDebugLastMessage("debug", string.Format("{0}|direct", currentMethodFullName)); | ||
|
||
} | ||
|
||
private async Task AsyncMethod2a() | ||
{ | ||
await AsyncMethod2b(); | ||
} | ||
private async Task AsyncMethod2b() | ||
{ | ||
var logger = LogManager.GetCurrentClassLogger(); | ||
logger.Warn("direct"); | ||
var reader = new StreamReader(new MemoryStream(new byte[0])); | ||
await reader.ReadLineAsync(); | ||
} | ||
|
||
|
||
[Fact] | ||
public void Show_correct_method_with_async3() | ||
{ | ||
|
||
//namespace en name of current method | ||
const string currentMethodFullName = "NLog.UnitTests.LayoutRenderers.CallSiteTests.AsyncMethod3b"; | ||
|
||
LogManager.Configuration = CreateConfigurationFromString(@" | ||
<nlog> | ||
<targets><target name='debug' type='Debug' layout='${callsite}|${message}' /></targets> | ||
<rules> | ||
<logger name='*' levels='Warn' writeTo='debug' /> | ||
</rules> | ||
</nlog>"); | ||
|
||
AsyncMethod3a().Wait(); | ||
AssertDebugLastMessage("debug", string.Format("{0}|direct", currentMethodFullName)); | ||
|
||
} | ||
|
||
private async Task AsyncMethod3a() | ||
{ | ||
var reader = new StreamReader(new MemoryStream(new byte[0])); | ||
await reader.ReadLineAsync(); | ||
AsyncMethod3b(); | ||
} | ||
private void AsyncMethod3b() | ||
{ | ||
var logger = LogManager.GetCurrentClassLogger(); | ||
logger.Warn("direct"); | ||
|
||
} | ||
|
||
#endif | ||
|
||
[Fact] | ||
public void Show_correct_method_for_moveNext() | ||
{ | ||
|
||
//namespace en name of current method | ||
const string currentMethodFullName = "NLog.UnitTests.LayoutRenderers.CallSiteTests.MoveNext"; | ||
|
||
LogManager.Configuration = CreateConfigurationFromString(@" | ||
<nlog> | ||
<targets><target name='debug' type='Debug' layout='${callsite}|${message}' /></targets> | ||
<rules> | ||
<logger name='*' levels='Warn' writeTo='debug' /> | ||
</rules> | ||
</nlog>"); | ||
|
||
MoveNext(); | ||
AssertDebugLastMessage("debug", string.Format("{0}|direct", currentMethodFullName)); | ||
|
||
} | ||
|
||
|
||
private void MoveNext() | ||
{ | ||
var logger = LogManager.GetCurrentClassLogger(); | ||
logger.Warn("direct"); | ||
|
||
} | ||
|
||
public class CompositeWrapper | ||
{ | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about comparing the type directly?
declaringType == typeof(System.Runtime.CompilerServices.AsyncTaskMethodBuilder)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's indeed better, thanks!