Yet another NodeCode problem affecting AsyncMemoize

[majocha]

It looks like any call to NodeCode.AwaitAsync (there are many in Transparent Compiler) is at risk of getting wrong threadstatics?

I've seen some randomness of that kind in #16701. Mostly the threadstatics get eventually restored at some point, but the potential for chaos is there.

I also added a modified AsyncMemoize test to show the problem.

[github-actions]

✅ No release notes required

[0101]

Wait, so are there wrong thread static in the async / task computation that we invoke from node, or do they also get corrupted when we're back in node? If it's the former we could probably say, if you want to use thread statics build phase / logger, you need to use node ?

Also, does switching to AsyncLocal fix this?

[majocha]

From my understanding, once we're in normal async, the flow of diagnotics state stops. So every time there is a thread switch in async, we end up with a wrong or null logger. This corrupts other running nodes, as long as there are any any diagnostics updates or, even worse, swapping of global diagnostics logger in such an async task.

I guess this is not very impactful because almost everything we have is wrapped in node.

And yes, AsyncLocal does not have that problem.

[majocha]

There are some things that further mask this problem. For example, we have implicit initialization if there happen to be a null logger in DiagnosticsThreadStatics getter.

I tried to remove this initialzation and swap it for explicit initialization at service boundary (any FCS service call sets defaults first). I then observed naked nulls showing up in AsyncMemoize.Get. So it seems the corruption sometimes gets through.

[0101]

Hmm, so basically we shouldn't have anything that is not node-wrapped touch the thread statics. Then it would be ok?

Anyway this seems like another good argument to switch to AsyncLocal.

[majocha]

Yes, that's exactly it. At least nothing that has potential to produce diagnostics or modify the global diagnostics logger should be executed.

[majocha]

One thing I noticed is ParseFile runs in normal async and can produce diagnostics.

[abonie]

Hey @majocha , is this PR good to go or are you planning to make some more improvements or tests?

[majocha]

This is more an issue than a PR, I will close this to not clutter things.