Debug proxy as external tool #19767

SteveSandersonMS · 2020-03-11T14:26:31Z

Implements #19702 and #19144

The list of commits makes this look more complicated than it is, because I tried out several approaches in sequence. It's only worth reviewing the final state. (Nonetheless it is still quite complicated, which is inherent to what we have to do here.)

One of the main design questions in this was how to get the debug proxy binaries into a place we can find them at runtime. I tried out all of the following approaches:

Actually having a compile-time reference from M.A.C.WA.Server to M.A.C.WA.DebugProxy. This means .NET Core itself will take care of getting the debug proxy assemblies into the output directory and locating them at runtime.
- However, this approach is bad because then the debug proxy's dependencies (including Newtonsoft.Json and Mono.Cecil) become real transitive dependencies of customers' "hosted" server projects, which makes no sense and could interfere if they want to use different versions of those packages.
Keeping the binaries inside the M.A.C.WA.DebugProxy package directory. I added some MSBuild stuff that, during build, writes out the location of that package directory as a file into the build output dir. At runtime we read that file.
- However, this approach is bad because it means you no longer have a self-contained build. You have a build that's tied to one machine and one OS. We had all sorts of problems with this before. Particularly in Docker scenarios (which aren't going to work out of the box anyway, but this approach would prevent you from making it work by enabling container->host networking).
Writing the binaries to the output directory in an independent subfolder.
- I think this is the most reasonable option because then you get a self-contained build and don't interfere with references. Also most of the mechanism happens automatically due to NuGet features and .NET build conventions (those binaries are just <Content> items in the M.A.C.WA.Server package).

If we want to change any aspects of this in the future we certainly can, but we won't have time to get too tied up about it for the preview 3 release.

Future follow-ups could include:

Moving the DebugProxy sources into the dotnet/blazor repo. If we did that, we could update the Mono runtime and its ws-proxy sources atomically using @pranavkm's automation.
Adding E2E tests covering actual debugging actions (i.e., automating clicks inside the Chrome dev tools debugger UI). That's kind of independent of this PR since it's something we could have done before and this PR doesn't change it. However the value to having them goes up now there are more moving parts involved. I'm not doing it now because it will be pretty expensive, and this feature has to ship almost immediately and we need to get on with the process of making builds and doing manual verification.

src/Components/WebAssembly/testassets/HostedInAspNet.Server/Properties/launchSettings.json

src/Components/WebAssembly/DebugProxy/src/Program.cs

pranavkm · 2020-03-11T15:22:17Z

src/Components/WebAssembly/DebugProxy/src/Startup.cs

+    {
+        public void Configure(IApplicationBuilder app, DebugProxyOptions debugProxyOptions)
+        {
+            app.UseDeveloperExceptionPage();


UseExceptionHandler?

I'm not sure why. The developer exception page provides useful diagnostic info, and this is a development-time feature only.

src/Components/WebAssembly/DebugProxy/src/Startup.cs

src/Components/WebAssembly/DebugProxy/src/TargetPickerUi.cs

pranavkm · 2020-03-11T15:43:56Z

src/Components/WebAssembly/Server/src/DebugProxyLauncher.cs

+            return await tcs.Task;
+        }
+
+        private static void RemoveUnwantedEnvironmentVariables(IDictionary<string, string> environment)


Can we have @Tratcher review this?

Yes but unless that happens particularly quickly, let's not block the initial merge on it.

It does make sense not to pass through the ASPNETCORE_* env vars (since we don't want the child process to try using the same ports as the parent, for example).

Do we have the list of environment variables before filtering? That would help us understand which ones we should remove and which ones we should preserve, but I believe they all should be removed.

You fully control the process being started, correct? In that case it would be cleaner to control the config from within that app. E.g. Don't use Host.CreateDefaultBuilder in that app and never add environment variables as a config source.

Thanks @Tratcher. We'll track your suggestions as follow up items

src/Components/WebAssembly/Server/src/DebugProxyLauncher.cs

pranavkm · 2020-03-11T15:51:15Z

src/Components/WebAssembly/Server/src/Microsoft.AspNetCore.Components.WebAssembly.Server.csproj

+  <Target Name="IncludeDebugProxyBinariesAsContent" BeforeTargets="GetCopyToOutputDirectoryItems">
+    <ItemGroup>
+      <DebugProxyBinaries Include="..\..\DebugProxy\src\bin\$(Configuration)\$(DefaultNetCoreTargetFramework)\**" />
+      <ContentWithTargetPath Include="@(DebugProxyBinaries)"


This feels very strange. Is there a reason the tool couldn't carry a copy of the proxy so it's not in the build output of the project?

See option 2 in the PR description.

src/Shared/CommandLineUtils/Utilities/DotNetMuxer.cs

SteveSandersonMS · 2020-03-12T12:00:56Z

@pranavkm @javiercn Don't let the build failures discourage you from approving this PR :) The build failures are a separate issue.

javiercn · 2020-03-12T15:51:08Z

Adding E2E tests covering actual debugging actions (i.e., automating clicks inside the Chrome dev tools debugger UI). That's kind of independent of this PR since it's something we could have done before and this PR doesn't change it. However the value to having them goes up now there are more moving parts involved. I'm not doing it now because it will be pretty expensive, and this feature has to ship almost immediately and we need to get on with the process of making builds and doing manual verification.

Do we need to setup CTI tests for this?

javiercn · 2020-03-12T15:59:58Z

src/Components/WebAssembly/Server/src/DebugProxyLauncher.cs

+            }
+        }
+
+        private static async Task<string> LaunchAndGetUrl(IServiceProvider serviceProvider)


Can I suggest that instead or relying on the Regexes (given that we control the proxy) we define on the server a "ready" endpoint and pass that down to the proxy and have the proxy send an Http request back to the server to indicate it's up and running? That way we avoid having to depend on regexes and reading console output and I think we would have a more reliable solution.

We can actually do this for keeping the process alive too, by passing in a "keep alive" endpoint that the proxy can ping to determine if the launcher process is still alive and terminate itself after a defined period of time if that's not the case. That way you don't just rely on the parent PID (which although is very rare, can be reused sometimes)

I'd like to not do that unless it becomes necessary. That would be yet more moving parts and a more complex polling system.

The approach of regexing console output is pretty commonplace for this purpose. We did it for NodeServices where it's continued to work fine for many years, plus we do it in our tests. VS Code does the same thing too when launching ASP.NET Core and connecting the debugger.

We don't need to keep the process alive. It stays alive on its own. We're using the parent's PID only when starting up the listener, and thereafter we attach to the Exited event which is designed for this scenario.

If any of this turns out to be inadequate in real use I'm very open to changing it, but current have no reason to think there are realistic drawbacks to this approach.

javiercn

I don't have a lot of context on the finer level details, but overall I agree with your solution and your changes look good.

If we end up not launching the debugger after May, then I'm not that worried about the specific mechanics we do for now.

…only, not a compile-time dependency in sources.

This reverts commit fa64728.

…ds polluting Solution Explorer.

Co-Authored-By: Pranav K <[email protected]>

SteveSandersonMS requested review from pranavkm and javiercn March 11, 2020 14:26

SteveSandersonMS commented Mar 11, 2020

View reviewed changes

src/Components/WebAssembly/testassets/HostedInAspNet.Server/Properties/launchSettings.json Show resolved Hide resolved

pranavkm reviewed Mar 11, 2020

View reviewed changes

mkArtakMSFT added area-blazor Includes: Blazor, Razor Components feature-blazor-wasm This issue is related to and / or impacts Blazor WebAssembly labels Mar 11, 2020

javiercn reviewed Mar 12, 2020

View reviewed changes

javiercn approved these changes Mar 12, 2020

View reviewed changes

SteveSandersonMS added 20 commits March 12, 2020 18:11

Begin adding webassembly-debugproxy tool

7ab64f9

Display list of debuggable pages

e6a58e5

Add actual ws-proxy logic

c7fb7b0

Tidy up

7763a94

Use the normal port

d938514

Strip obsolete things from M.A.C.WA.Server project

cfd12a8

Add project reference

750211f

Emit info about debug proxy location to bin during build

3d147bd

Produce NuGet package

d77044c

Obtain debug proxy config path in dev server

88bfd3f

Remove project reference. Want to model this as a package dependency …

382c9fd

…only, not a compile-time dependency in sources.

Copy actual binaries to output dir

6f3353b

Revert "Copy actual binaries to output dir"

88ad0f5

This reverts commit fa64728.

Clean up build target locations

cfa6651

Simplify: include dev server as content files

549e82e

Revert redundant changes

ec13eef

Fix builds from clean state

22132f2

Redirect to debug proxy

e7a9a87

Actually launch debug proxy

b33a6f3

Fix URLs

37214f2

SteveSandersonMS and others added 18 commits March 12, 2020 18:11

Fix launcher

88531c7

Include testassets launchsettings so it's easier to test debugging

2b3f05c

Determine port dynamically

d2fcf1e

Quit when parent quits

64cbbde

Fix package layoua

f07fe47

Fix layout again

963c6bf

Another layout attempt

d898af7

Use custom build targets to copy debug proxy binaries to output. Avoi…

549e6a3

…ds polluting Solution Explorer.

Make it skippable if desired

2f12529

CR: Use Process.Exited event

a1c585c

CR: Clean up

1354201

Update src/Components/WebAssembly/DebugProxy/src/Startup.cs

9f25b0b

Co-Authored-By: Pranav K <[email protected]>

Update src/Components/WebAssembly/Server/src/DebugProxyLauncher.cs

952f7c5

Co-Authored-By: Pranav K <[email protected]>

CR: License header

5e32dc8

CR: Detect application startup when it says "Application started"

92131da

CR: Just use Assembly.Location

dbe960d

Fix typo

8580a77

Tidy

b031b74

SteveSandersonMS force-pushed the stevesa/debug-proxy-as-external-tool branch from 40d0e40 to b031b74 Compare March 12, 2020 18:12

SteveSandersonMS merged commit 2e9bb2f into blazor-wasm Mar 12, 2020

SteveSandersonMS deleted the stevesa/debug-proxy-as-external-tool branch March 12, 2020 18:42

This was referenced Mar 13, 2020

Remove obsolete VS debugging endpoints from Blazor WebAssembly #19144

Closed

Move debug proxy to external process #19702

Closed

pranavkm mentioned this pull request Mar 16, 2020

Consider not using CreateDefaultBuilder in Blazor debugger-proxy #19909

Closed

Debug proxy as external tool #19767

Debug proxy as external tool #19767

Uh oh!

Conversation

SteveSandersonMS commented Mar 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SteveSandersonMS Mar 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

SteveSandersonMS commented Mar 12, 2020

Uh oh!

javiercn commented Mar 12, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SteveSandersonMS Mar 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

javiercn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

SteveSandersonMS commented Mar 11, 2020 •

edited

Loading

SteveSandersonMS Mar 12, 2020 •

edited

Loading

SteveSandersonMS Mar 12, 2020 •

edited

Loading