NRE on Dispose in ExecutionTimer

[dee-see]

I'm breakpointing through the PSES to figure out how the Find References mechanism works for another issue and I'm getting NRE in ExecutionTimer quite frequently.

The NRE happens on the first line of the Dispose() method.

t_stopwatch.Stop();

It's happening here and in other similar blocks of code

using (Logger.LogExecutionTime($"Script analysis of {scriptFile.FilePath} completed."))
{
    semanticMarkers = await editorSession.AnalysisService.GetSemanticMarkersAsync(scriptFile);
}

Basically the ThreadStatic t_stopwatch field is assigned inside the Logger.LogExecutionTime call, but when we come back from the 'await' the code is/can be executed on another thread and Logger.LogExecutionTime is either null, the right t_stopwatch or one from another thread.

So before I submit a PR making t_stopwatch an instance field I wanted to ask why is it ThreadStatic? Am I going to break everything in a way I'm not seeing right now?

Thanks

(I'm running the application on Arch Linux, building with VS Code)

[rkeithhill]

Hmm, unless someone messes with the SynchronizationContext, the continuation "should" happen on the thread that started the await.

[dee-see]

Hmm ok I see. I'll try to write a unit test that reproduces the issue and figure out more precisely what's going on. Maybe it's a platform issue, I'll see if it behaves the same way on Windows.

EDIT: For what it's worth I'm getting different threads before and after the await in this fiddle https://dotnetfiddle.net/VSDAzA

[rjmholt]

The intent of the [ThreadStatic] attribute was to prevent needing to do any thread handling on the stopwatch object. Since a stopwatch is expensive to create, I figured we'd have a static one. But to prevent thread contention that wouldn't exist otherwise, I made it threadstatic. I was hoping that we could keep the overhead of adding a stopwatch to a minimum.

However it sounds like I haven't accounted for managing async continuations moving to new threads properly. Given the choice between locking and creating a fresh stopwatch every time, I'd prefer to create the stopwatch. But otherwise, perhaps a "stopwatch pool" is in order?

[dee-see]

Yeah unless we're timing a lot of things at the same time the pool might end up creating fewer stopwatches than the ThreadStatic implementation did.

Thanks for the information, I'll work on something to deal with the issue.