Skip to content

Fix a couple of issues with dotnet-watch #17608

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

Merged
merged 1 commit into from
May 13, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion src/BuiltInTools/DotNetDeltaApplier/HotReloadAgent.cs
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ internal class HotReloadAgent : IDisposable
{
private readonly Action<string> _log;
private readonly AssemblyLoadEventHandler _assemblyLoad;
private readonly ConcurrentDictionary<Guid, IReadOnlyList<UpdateDelta>> _deltas = new();
private readonly ConcurrentDictionary<Guid, List<UpdateDelta>> _deltas = new();
private readonly ConcurrentDictionary<Assembly, Assembly> _appliedAssemblies = new();
private volatile UpdateHandlerActions? _handlerActions;

Expand Down Expand Up @@ -196,6 +196,10 @@ public void ApplyDeltas(IReadOnlyList<UpdateDelta> deltas)
{
System.Reflection.Metadata.AssemblyExtensions.ApplyUpdate(assembly, item.MetadataDelta, item.ILDelta, ReadOnlySpan<byte>.Empty);
}

// Additionally stash the deltas away so it may be applied to assemblies loaded later.
var cachedDeltas = _deltas.GetOrAdd(item.ModuleId, static _ => new());
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We were applying these deltas, but I missed storing them.

cachedDeltas.Add(item);
}

handlerActions.ClearCache.ForEach(a => a(updatedTypes));
Expand Down
5 changes: 0 additions & 5 deletions src/BuiltInTools/DotNetDeltaApplier/StartupHook.cs
Original file line number Diff line number Diff line change
Expand Up @@ -53,11 +53,6 @@ public static async Task ReceiveDeltas(HotReloadAgent hotReloadAgent)
Log("Attempting to apply deltas.");

hotReloadAgent.ApplyDeltas(update.Deltas);

// We want to base this off of mvids, but we'll figure that out eventually.
var applyResult = update.ChangedFile is string changedFile && changedFile.EndsWith(".razor", StringComparison.Ordinal) ?
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed the heuristic about when we refresh the browser in web apps - we won't it if it looks like a Blazor app (presence of any .razor file). Works better than relying on individual files.

ApplyResult.Success :
ApplyResult.Success_RefreshBrowser;
pipeClient.WriteByte((byte)ApplyResult.Success);

}
Expand Down
12 changes: 10 additions & 2 deletions src/BuiltInTools/dotnet-watch/HotReload/CompilationHandler.cs
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,8 @@ public async ValueTask<bool> TryHandleFileChange(DotNetWatchContext context, Fil
{
_reporter.Verbose("No deltas modified. Applying changes to clear diagnostics.");
await _deltaApplier.Apply(context, file.FilePath, updates, cancellationToken);
// Even if there were diagnostics, continue treating this as a success
_reporter.Output("No hot reload changes to apply.");
}
else
{
Expand All @@ -123,7 +125,8 @@ public async ValueTask<bool> TryHandleFileChange(DotNetWatchContext context, Fil
}

HotReloadEventSource.Log.HotReloadEnd(HotReloadEventSource.StartType.CompilationHandler);
// Even if there were diagnostics, continue treating this as a success
// Return true so that the watcher continues to keep the current hot reload session alive. If there were errors, this allows the user to fix errors and continue
// working on the running app.
return true;
}

Expand All @@ -145,6 +148,11 @@ public async ValueTask<bool> TryHandleFileChange(DotNetWatchContext context, Fil
var applyState = await _deltaApplier.Apply(context, file.FilePath, updates, cancellationToken);
_reporter.Verbose($"Received {(applyState ? "successful" : "failed")} apply from delta applier.");
HotReloadEventSource.Log.HotReloadEnd(HotReloadEventSource.StartType.CompilationHandler);
if (applyState)
{
_reporter.Output($"Hot reload of changes succeeded.");
}

return applyState;
}

Expand All @@ -171,7 +179,7 @@ private ImmutableArray<string> GetDiagnostics(Solution solution, CancellationTok
if (item.Severity == DiagnosticSeverity.Error)
{
var diagnostic = CSharpDiagnosticFormatter.Instance.Format(item);
_reporter.Output(diagnostic);
_reporter.Output("\x1B[40m\x1B[31m" + diagnostic);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pretty red color

projectDiagnostics = projectDiagnostics.Add(diagnostic);
}
}
Expand Down
26 changes: 16 additions & 10 deletions src/BuiltInTools/dotnet-watch/HotReload/DefaultDeltaApplier.cs
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,11 @@ namespace Microsoft.DotNet.Watcher.Tools
{
internal class DefaultDeltaApplier : IDeltaApplier
{
private static readonly string _namedPipeName = Guid.NewGuid().ToString();
private readonly IReporter _reporter;
private readonly string _namedPipeName = Guid.NewGuid().ToString();
private Task _task;
private NamedPipeServerStream _pipe;
private bool _refreshBrowserAfterFileChange;

public DefaultDeltaApplier(IReporter reporter)
{
Expand All @@ -29,14 +30,8 @@ public DefaultDeltaApplier(IReporter reporter)

public bool SuppressBrowserRefreshAfterApply { get; init; }

public async ValueTask InitializeAsync(DotNetWatchContext context, CancellationToken cancellationToken)
public ValueTask InitializeAsync(DotNetWatchContext context, CancellationToken cancellationToken)
{
if (_pipe is not null)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why was this removed? It seems like good hygiene to have it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Turns out we were newing up these instances in every hot reload loop. So this was never true. Also why the named pipe name is a static because we were only configuring it once.

{
_pipe.Close();
await _pipe.DisposeAsync();
}

_pipe = new NamedPipeServerStream(_namedPipeName, PipeDirection.InOut, 1, PipeTransmissionMode.Byte, PipeOptions.Asynchronous | PipeOptions.CurrentUserOnly);
_task = _pipe.WaitForConnectionAsync(cancellationToken);

Expand All @@ -48,7 +43,15 @@ public async ValueTask InitializeAsync(DotNetWatchContext context, CancellationT
// Configure the app for EnC
context.ProcessSpec.EnvironmentVariables["DOTNET_MODIFIABLE_ASSEMBLIES"] = "debug";
context.ProcessSpec.EnvironmentVariables["DOTNET_HOTRELOAD_NAMEDPIPE_NAME"] = _namedPipeName;

// If there's any .razor file, we'll assume this is a blazor app and not cause a browser refresh.
if (!SuppressBrowserRefreshAfterApply)
{
_refreshBrowserAfterFileChange = !context.FileSet.Any(f => f.FilePath.EndsWith(".razor", StringComparison.Ordinal));
}
}

return default;
}

public async ValueTask<bool> Apply(DotNetWatchContext context, string changedFile, ImmutableArray<WatchHotReloadService.Update> solutionUpdate, CancellationToken cancellationToken)
Expand Down Expand Up @@ -110,11 +113,14 @@ public async ValueTask<bool> Apply(DotNetWatchContext context, string changedFil

if (!SuppressBrowserRefreshAfterApply && context.BrowserRefreshServer is not null)
{
if (result == ApplyResult.Success_RefreshBrowser)
// For a Web app, we have the option of either letting the app update the UI or
// refresh the browser. In general, for Blazor apps, we will choose not to refresh the UI
// and for other apps we'll always refresh
if (_refreshBrowserAfterFileChange)
{
await context.BrowserRefreshServer.ReloadAsync(cancellationToken);
}
else if (result == ApplyResult.Success)
else
{
await context.BrowserRefreshServer.SendJsonSerlialized(new HotReloadApplied());
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,5 @@ internal enum ApplyResult
{
Failed = -1,
Success = 0,
Success_RefreshBrowser = 1,
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ public async ValueTask<bool> TryHandleFileChange(DotNetWatchContext context, Fil
return false;
}
await HandleBrowserRefresh(context.BrowserRefreshServer, file, cancellationToken);
_reporter.Output("Hot reload of scoped css succeeded.");
HotReloadEventSource.Log.HotReloadEnd(HotReloadEventSource.StartType.ScopedCssHandler);
return true;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ public async ValueTask<bool> TryHandleFileChange(DotNetWatchContext context, Fil
_reporter.Verbose($"Handling file change event for static content {file.FilePath}.");
await HandleBrowserRefresh(context.BrowserRefreshServer, file, cancellationToken);
HotReloadEventSource.Log.HotReloadEnd(HotReloadEventSource.StartType.StaticHandler);
_reporter.Output("Hot reload of static file succeeded.");
return true;
}

Expand Down
3 changes: 1 addition & 2 deletions src/BuiltInTools/dotnet-watch/HotReloadDotNetWatcher.cs
Original file line number Diff line number Diff line change
Expand Up @@ -128,8 +128,7 @@ public async Task WatchAsync(DotNetWatchContext context, CancellationToken cance
if (await hotReload.TryHandleFileChange(context, fileItem, combinedCancellationSource.Token))
{
var totalTime = TimeSpan.FromTicks(Stopwatch.GetTimestamp() - start);
_reporter.Output($"Hot reload of changes succeeded.");
_reporter.Verbose($"Hot reload applied in {totalTime.TotalMilliseconds}ms.");
_reporter.Verbose($"Hot reload change handled in {totalTime.TotalMilliseconds}ms.");
}
else
{
Expand Down
6 changes: 6 additions & 0 deletions src/BuiltInTools/dotnet-watch/Program.cs
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,13 @@ public Program(IConsole console, string workingDirectory)
// AppContext.BaseDirectory = $sdkRoot\$sdkVersion\DotnetTools\dotnet-watch\$version\tools\net6.0\any\
// MSBuild.dll is located at $sdkRoot\$sdkVersion\MSBuild.dll
var sdkRootDirectory = Path.Combine(AppContext.BaseDirectory, "..", "..", "..", "..", "..", "..");
#if DEBUG
// In the usual case, use the SDK that contains the dotnet-watch. However during local testing, it's
// much more common to run dotnet-watch from a different SDK. Use the ambient SDK in that case.
MSBuildLocator.RegisterDefaults();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Annoying local build issue where it would use the dogfood SDK. This was causing the annoying FailFast error that I poked you about.

#else
MSBuildLocator.RegisterMSBuildPath(sdkRootDirectory);
#endif

Ensure.NotNull(console, nameof(console));
Ensure.NotNullOrEmpty(workingDirectory, nameof(workingDirectory));
Expand Down