Only default to attaching debugger if aspire itself is under debug

[twsouthwick]

This allows for the common case that when running under debug, things should debug. However, if things are not under debug, probably don't need to attach the debugger. However, if a user wants to force it to, they can still call the .WithDebugger() method manually.

[CZEMacLeod]

I think this check could be rolled into

C3D.Extensions.Aspire/src/C3D/Extensions/Aspire/VisualStudioDebug/Extensions/DebugResourceExtensions.cs

Lines 12 to 20 in 71717d4

	public static bool IsDebugMode<TResource>(this IResourceBuilder<TResource> resourceBuilder, params string[]? environments)
	where TResource : IResource =>
	(resourceBuilder as IDebugBuilder<TResource>)?.IsDebugMode ?? (
	resourceBuilder.ApplicationBuilder.ExecutionContext.IsRunMode &&
	((environments is null || environments.Length == 0) ?
	resourceBuilder.ApplicationBuilder.Environment.IsDevelopment() :
	environments.Any(resourceBuilder.ApplicationBuilder.Environment.IsEnvironment)
	) &&
	!resourceBuilder.IsUnderTest());

Also, it should not actually attach the debugger if it can't find the instance of visual studio running attached to the aspire host project.

C3D.Extensions.Aspire/src/C3D/Extensions/Aspire/VisualStudioDebug/AttachDebuggerHook.cs

Lines 159 to 165 in 71717d4

	var aspireHost = Process.GetCurrentProcess();
	var vs = await visualStudioInstances.GetAttachedVisualStudioAsync(aspireHost);
	if (vs is null)
	{
	logger.LogError("Could not get debugger for aspire host {host}", aspireHost);
	return;
	}

But it does largely make sense to not add the annotation if the debugger is not attached when setting up the resource.

    public static bool IsDebugMode<TResource>(this IResourceBuilder<TResource> resourceBuilder, params string[]? environments)
        where TResource : IResource =>
            (resourceBuilder as IDebugBuilder<TResource>)?.IsDebugMode ?? (
            System.Diagnostics.Debugger.IsAttached &&
            resourceBuilder.ApplicationBuilder.ExecutionContext.IsRunMode &&
            ((environments is null || environments.Length == 0) ?
                resourceBuilder.ApplicationBuilder.Environment.IsDevelopment() :
                environments.Any(resourceBuilder.ApplicationBuilder.Environment.IsEnvironment)
                ) &&
            !resourceBuilder.IsUnderTest());

This will make the change affect all project types - e.g. NodeJS, Python, PHP, etc. as well as IIS - and mean we don't have to adjust each resource types' extension method.

[twsouthwick]

You're right - it's not that it's attaching (although an error is weird if I'm not attached myself to the debugger), the issue I was hitting was that a default debug health check was added and therefore never is "healthy".

Thoughts?

[CZEMacLeod]

Well, WithDebuggerHealthcheck checks for IsDebugMode on the IDebugBuilder, which would be set to false if you add the attached check to the code that initialises the debugger builder. The best way of doing that is the change I suggested to IsDebugMode on IResourceBuilder which will mean than none of the debug stuff initialises.

Can I ask what the scenario you were hitting this in was, as the debug stuff should also be disabled under test mode (which checks for "Aspire.Hosting.Testing" in the stack trace) which should be the case when automating an apphost under test.
I guess if you tried to run it from the command line (aspire cli) it might hit this, but I'm not sure if there is an easy way to detect that (other than the debugger attached we are already discussing).

[CZEMacLeod]

With #33 and the change I suggested above, it now works from the Aspire CLI using aspire run.
I had been experimenting with trying to do something on publish and testing from the command line so this was a pretty easy thing to check.

[twsouthwick]

My scenario was just running dotnet run from the command line on the apphost.

Any of the samples on this branch of the adapters: https://github.com/dotnet/systemweb-adapters/tree/build-samples

[twsouthwick]

btw still working through getting the webapplications.targets relicensed....

[CZEMacLeod]

Okay - my suggested change works for dotnet run.

I've published an unlisted packages which support this:

<PackageReference Include="C3D.Extensions.Aspire.VisualStudioDebug" Version="0.1.42-g0f2568ab6a" />
<PackageReference Include="C3D.Extensions.Aspire.Fluent" Version="0.2.7-g0f2568ab6a" />
<PackageReference Include="C3D.Extensions.VisualStudioDebug" Version="0.1.27-g0f2568ab6a" />

Changes are on my fix-aspire-debuggerattached branch.

[CZEMacLeod]

@twsouthwick Let me know if those work for you, and we can either change this PR to match my branch, or we can close this, and I'll merge my branch.

[twsouthwick]

Yup your change works so that's good enough for me. I'll close this change and you can merge yours.

One nit is that it still appears to be attempting to attach the debugger:

[CZEMacLeod]

The message is emitted on the BackgroundService starting, where it waits for resources to start and checks if they have the appropriate annotations.
In this case the resources don't have the annotation, so nothing happens, but the AttachDebuggerHook is still running, waiting for any processes it should debug.
I've removed the redundant calls to AddAttachDebuggerHook in AddIISExpress and AddIISExpressConfiguration as when you add the project, either through AddSiteProject or AddIISExpressProject, the call to WithDebugger will call that anyway - after it checks for IsDebugMode.
This should mean that it won't be added and run in the background if no resource actually has the appropriate annotation.

[CZEMacLeod]

@twsouthwick Out of interest, before we close this, do you have any idea why the tests are sometimes failing with a wierd polly timeout of 10s (when my tests all setup aspire to have polly timeouts of 30/60/120s).
Polly.Timeout.TimeoutRejectedException : The operation didn't complete within the allowed timeout of '00:00:10'.
You should see it in the github action (and it also occasionally does it on the Azure DevOps hosted agents. Re-running it normally clears the error....

[twsouthwick]

I'm not sure - I've seen random failures (not always with output) that I'm investigating as well.