NullReferenceException on stopwatch
GeertvanHorrik opened this issue · 24 comments
This happens on an async method that is nested. Going to check if I can reproduce. This code has always worked, just updated to latest version of MethodTimer and it fails now.
updated to latest version
Updated from what version?
VS2015?
1.15.3 => 1.15.5.0 (both NET40).
It doesn't happen with all methods. I just found one. Will try to find out what has happened. Will investigate if more has changed that might impact this.
And of course I cannot reproduce now. I am glad I took the screenshot so at least I have some evidence ;-)
I was thinking that it might be happening when fast-path optimization takes place in a method like this:
private async Task UpdateAsync()
{
if (_isUpdating)
{
return; // <= fast optimization here
}
await _myService.DoMagicAsync();
}
Yep, that's it!
Going to create a repro + pr.
awesome. thanks
2 unit tests seem to fail?
VS2015?
Yep.
ok. then i know what it is. give me 15
This is the (broken) generated IL on NET40 / VS 2015 for the bug. See how the default allows an exit (and stops the timer) when the timer is not yet instantiated.
private void MoveNext()
{
int num = this.<>1__state;
try
{
MainViewModel.<UpdateOperationsAsync>d__20 d__;
switch (num)
{
case 0:
break;
case 1:
goto Label_01E6;
default:
if (this.<>4__this._isRefreshing)
{
this.<>4__this._isRefreshingQueued = true;
this.methodTimerStopwatch.Stop();
MethodTimeLogger.Log(methodof(MainViewModel.UpdateOperationsAsync, MainViewModel), this.methodTimerStopwatch.ElapsedMilliseconds);
goto Label_025B;
}
this.<>4__this._isRefreshing = true;
this.<operations>5__1 = this.<>4__this.Operations;
this.<>s__2 = this.<operations>5__1.SuspendChangeNotifications();
break;
}
this.methodTimerStopwatch = Stopwatch.StartNew();
try
{
// rest of the method
fixed the broken test
When I compare it with my newly written unit tests, it uses a different try / catch mechanism. Probably the stopwatch is injected before the first try (that is consistent), but in my app it is the wrong one.
Might be caused by this commit:
I think we can safely set the stopwatch after the first usage of a variable containing "__state".
looks good to me
Fix seems to work on my software as well. Creating PR now so you can review.
ok. I will smoke test and deploy
No, we must be more careful. This will reset the stopwatch. We must check if the stopwatch is not already running.
We must use code like this:
if (_stopWatch == null)
{
_stopWatch = System.Diagnostics.Stopwatch.StartNew();
}
Give me 5 more minutes
This is the newly generated code in my app:
private void MoveNext()
{
if (this.methodTimerStopwatch == null)
{
this.methodTimerStopwatch = Stopwatch.StartNew();
}
int num = this.<>1__state;
try
{
MainViewModel.<UpdateOperationsAsync>d__20 d__;
switch (num)
{
case 0:
break;
case 1:
goto Label_01F3;
default:
if (this.<>4__this._isRefreshing)
{
this.<>4__this._isRefreshingQueued = true;
this.methodTimerStopwatch.Stop();
I think the "overhead" for checking null is acceptable. Just need to ensure that we don't start a stopwatch when the state machine has been completed.
I also noticed that if there is an exception, the timer does not reset.
Written unit tests for the exception scenarios as well as a fix.
See PR #21