Avoid trying to use _hotReloadAgent variable before it is initialized #46916
Conversation
ghost
added
the
| @@ -50,14 +45,22 @@ internal static async Task InitializeAsync() | |||
| [JSInvokable(nameof(ApplyHotReloadDelta))] | |||
| public static void ApplyHotReloadDelta(string moduleIdString, byte[] metadataDelta, byte[] ilDelta, byte[] pdbBytes) | |||
| { | |||
| // Analyzer has a bug where it doesn't handle ConditionalAttribute: https://github.com/dotnet/roslyn/issues/63464 | |||
| #pragma warning disable IDE0200 // Remove unnecessary lambda expression | |||
| if (_hotReloadAgent == null) | |||
There was a problem hiding this comment.
Shouldn't this be Interlocked.CompareExchange to avoid multiple threads ending up with different instances?
There was a problem hiding this comment.
I think that if it has multiple initialization, it is not an issue for the runtime.
But I will change as you suggested to avoid any issue.
| @@ -50,14 +45,19 @@ internal static async Task InitializeAsync() | |||
| [JSInvokable(nameof(ApplyHotReloadDelta))] | |||
| public static void ApplyHotReloadDelta(string moduleIdString, byte[] metadataDelta, byte[] ilDelta, byte[] pdbBytes) | |||
| { | |||
| // Analyzer has a bug where it doesn't handle ConditionalAttribute: https://github.com/dotnet/roslyn/issues/63464 | |||
| #pragma warning disable IDE0200 // Remove unnecessary lambda expression | |||
| Interlocked.CompareExchange(ref _hotReloadAgent, new HotReloadAgent(m => Debug.WriteLine(m)), null); | |||
There was a problem hiding this comment.
Will a new HotReloadAgent be initialized even in case if the value shouldn't be replaced here? I assume the compiler is smart enough not to do that, but I'm not sure, hence my question.
There was a problem hiding this comment.
Interlocked.CompareExchange is thread safe and will avoid creating two HotReloadAgent in two different threads.
It compares two values for equality and, if they are equal, replaces the first value.
| @@ -50,14 +45,19 @@ internal static async Task InitializeAsync() | |||
| [JSInvokable(nameof(ApplyHotReloadDelta))] | |||
| public static void ApplyHotReloadDelta(string moduleIdString, byte[] metadataDelta, byte[] ilDelta, byte[] pdbBytes) | |||
| { | |||
| // Analyzer has a bug where it doesn't handle ConditionalAttribute: https://github.com/dotnet/roslyn/issues/63464 | |||
| #pragma warning disable IDE0200 // Remove unnecessary lambda expression | |||
| Interlocked.CompareExchange(ref _hotReloadAgent, new HotReloadAgent(m => Debug.WriteLine(m)), null); | |||
There was a problem hiding this comment.
Why does the initialization in the InitializeAsync method is not enough? Does the value gets discarded through some other codepath?
There was a problem hiding this comment.
As the InitializeAsync is an async method what was happening is that in a race condition before _hotReloadAgent is initialized on InitializeAsync it was trying to be used on ApplyHotReloadDelta method and throwing the NRE exception.
There was a problem hiding this comment.
That sounds like a bug in the consuming code then, rather than here, isn't it? So the consumder then should just make sure they wait for the initialization before trying to apply changes.
There was a problem hiding this comment.
I'm not sure if it's possible to check if InitializeAsync is done before calling the ApplyHotReloadDelta as it's called by dotnet watch.
There was a problem hiding this comment.
It is possible that there is an issue with initialization. I am not sure what exactly the initialization sequence for WASM is supposed to do. It'd be great if anyone knows and can review the code paths.
There was a problem hiding this comment.
Here is where InitializeAsync is called:
And here is where it's calling the applyHotReload function on JS from dotnet watch:
https://github.com/dotnet/sdk/blob/fbc089ea96f0187c31e9ae176278334c8d5defc6/src/BuiltInTools/BrowserRefresh/BlazorHotReload.js#L16
Which will call the ApplyHotReloadDelta on C# side:
I think there is no way to guarantee that InitializeAsync is done before calling the ApplyHotReloadDelta, I guess that this is the correct fix.
There was a problem hiding this comment.
Discussed my comments with @thaystg.
Looks like fixing the initialization code on the dotnet-watch is a compromise which had been thought about already.
Fixes #45190