Implement package testing in ASP.NET Core repo

[rquackenbush]

Is there an existing issue for this?

• I have searched the existing issues

Describe the bug

Adding health checks with AddHealthChecks().AddCheck() prevents calls to IHealthCheckPublisher.PublishAsync.

Expected Behavior

I expect IHealthCheckPublisher.PublishAsync to be called where .AddCheck has been called or not.

Steps To Reproduce

https://github.com/anvilcloud/HealthChecksRepro

Exceptions (if any)

None.

.NET Version

7.0.100

Anything else?

IDE
Microsoft Visual Studio Professional 2022 (64-bit) - Current
Version 17.4.2

dotnet --info

.NET SDK:
 Version:   7.0.100
 Commit:    e12b7af219

Runtime Environment:
 OS Name:     Windows
 OS Version:  10.0.19044
 OS Platform: Windows
 RID:         win10-x64
 Base Path:   C:\Program Files\dotnet\sdk\7.0.100\

Host:
  Version:      7.0.0
  Architecture: x64
  Commit:       d099f075e4

.NET SDKs installed:
  7.0.100 [C:\Program Files\dotnet\sdk]

.NET runtimes installed:
  Microsoft.AspNetCore.All 2.1.30 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.All]
  Microsoft.AspNetCore.App 2.1.30 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 3.1.31 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 6.0.11 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 7.0.0 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.NETCore.App 2.1.30 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 3.1.31 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 5.0.17 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 6.0.9 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 6.0.11 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 7.0.0 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.WindowsDesktop.App 3.1.31 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
  Microsoft.WindowsDesktop.App 6.0.11 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
  Microsoft.WindowsDesktop.App 7.0.0 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]

Other architectures found:
  x86   [C:\Program Files (x86)\dotnet]
    registered at [HKLM\SOFTWARE\dotnet\Setup\InstalledVersions\x86\InstallLocation]

Environment variables:
  Not set

global.json file:
  Not found

Learn more:
  https://aka.ms/dotnet/info

Download .NET:
  https://aka.ms/dotnet/download

Update: This works if the TargetFramework is set to net7.0. Unfortunately I need to do this in net6.0.

[rquackenbush]

Downgrading the HealthChecks packages worked for me

<PackageReference Include="Microsoft.Extensions.Diagnostics.HealthChecks.Abstractions" Version="6.0.14" />
<PackageReference Include="Microsoft.Extensions.Diagnostics.HealthChecks" Version="6.0.14" />

[halter73]

Thanks for the repro. You're issue seems to be mainly that you're mixing Microsoft.Extensions.Diagnostics.HealthChecks 7.0.3 with a net6.0 target framework. This forces you to use it as a netstandard2.0 dependency since there's only a net7.0 and no net6.0 dependency.

When used as a netstandard2.0 dependency on net6.0, you apparently need Microsoft.Bcl.AsyncInterfaces because we call DisposeAsync in DefaultHealthCheckService.RunCheckAsync, but RunCheckAsync gets skipped if there are no registrations.

aspnetcore/src/HealthChecks/HealthChecks/src/DefaultHealthCheckService.cs

Lines 78 to 81 in 63a0251

	var scope = _scopeFactory.CreateAsyncScope();
	await using (scope.ConfigureAwait(false))
	{
	var healthCheck = registration.Factory(scope.ServiceProvider);

System.IO.FileNotFoundException: 'Could not load file or assembly 'Microsoft.Bcl.AsyncInterfaces, Version=7.0.0.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51'. The system cannot find the file specified.'
   at Microsoft.Extensions.Diagnostics.HealthChecks.DefaultHealthCheckService.RunCheckAsync(HealthCheckRegistration registration, CancellationToken cancellationToken)
   at System.Threading.Tasks.Task`1.InnerInvoke() in /_/src/libraries/System.Private.CoreLib/src/System/Threading/Tasks/Future.cs:line 503

aspnetcore/src/HealthChecks/HealthChecks/src/DefaultHealthCheckService.cs

Line 79 in 63a0251

await using (scope.ConfigureAwait(false))

For now, you can work around this issue by either.

1. Upgrading your app to target net7.0, so Microsoft.Extensions.Diagnostics.HealthChecks can be used as a net7.0 depencency.
2. Downgrading your Microsoft.Extensions.Diagnostics.HealthChecks dependency to 6.0.14, so it can be used as a net6.0 dependency.
3. Manually adding the <PackageReference Include="Microsoft.Bcl.AsyncInterfaces" Version="7.0.0" /> package reference that Microsoft.Extensions.Diagnostics.HealthChecks probably should have added transitively for you.

Our recommendation is always to try to keep versions aligned if possible since that will keep you on the most well-tested path, but I still think we should probably add Microsoft.Bcl.AsyncInterfaces as a netstandard2.0 dependency for this package.

@dotnet/dnceng @dotnet/aspnet-build. Is there some way to automatically detect we're using IAsyncDisposable or IAsyncEnumerable to a NuGet package targeting netstandard2.0 and fail the build if there's no Microsoft.Bcl.AsyncInterfaces dependency?

[halter73]

@rquackenbush Thanks for the bug report and posting the resolution yourself, I'm reopening this to track the following question:

@dotnet/dnceng @dotnet/aspnet-build. Is there some way to automatically detect we're using IAsyncDisposable or IAsyncEnumerable to a NuGet package targeting netstandard2.0 and fail the build if there's no Microsoft.Bcl.AsyncInterfaces dependency?

[alexperovich]

@dotnet/dnceng @dotnet/aspnet-build. Is there some way to automatically detect we're using IAsyncDisposable or IAsyncEnumerable to a NuGet package targeting netstandard2.0 and fail the build if there's no Microsoft.Bcl.AsyncInterfaces dependency?

You shouldn't be able to compile code using them without the reference. I'm surprised the package compiled in the netstandard2.0 trm.

[alexperovich]

The result should be one of the "Missing Compiler required member" errors.

[dougbu]

@rquackenbush Thanks for the bug report and posting the resolution yourself, I'm reopening this to track the following question:

@dotnet/dnceng @dotnet/aspnet-build. Is there some way to automatically detect we're using IAsyncDisposable or IAsyncEnumerable to a NuGet package targeting netstandard2.0 and fail the build if there's no Microsoft.Bcl.AsyncInterfaces dependency?

@dotnet/nuget-team sorry for the broad notification. Would appreciate your thoughts on any options that may be available. My only thought would involve complicated msbuild bits in a Microsoft.Extensions.Diagnostics.HealthChecks.targets file. That would only cover this specific issue and hunting for all IAsyncDisposable code in our repo would also ignore all other dotnet/* code.

To @alexperovich's point, it's not clear how compilation succeeded. Has anyone gathered a binary log to help us (or the Roslyn team) figure that part out❔

[alexperovich]

This should be easily fixed with a <PackageReference Condition="'$(TargetFramework)' == 'netstandard2.0'" Include="Microsoft.Bcl.AsyncInterfaces" />

[dougbu]

This should be easily fixed with a <PackageReference Condition="'$(TargetFramework)' == 'netstandard2.0'" Include="Microsoft.Bcl.AsyncInterfaces" />

Yeah, my "complicated msbuild bits" comment was a wild exaggeration. Still worried about the general issue of finding all the packages we ship that might hit similar issues.

[alexperovich]

The code compiling is the real problem. It should fail building.

[dougbu]

Let's get a binary log (someone❔) and bring in @jaredpar to get his opinion.

[dougbu]

Our local builds of Microsoft.Extensions.Diagnostics.HealthChecks.csproj work because we have a transitive reference (through Microsoft.Extensions.Hosting.Abstractions) when building for netstandard2.0. The Microsoft.Extensions.Hosting.Abstractions dependency also shows up in the Microsoft.Extensions.Hosting.Abstractions package. There's no reason this wouldn't happen in the repro project as well.

Put another way, the answer to

https://github.com/orgs/dotnet/teams/dnceng https://github.com/orgs/dotnet/teams/aspnet-build. Is there some way to automatically detect we're using IAsyncDisposable or IAsyncEnumerable to a NuGet package targeting netstandard2.0 and fail the build if there's no Microsoft.Bcl.AsyncInterfaces dependency?

is that the package metadata should already bring in Microsoft.Bcl.AsyncInterfaces when using the netstandard2.0 assembly from this package.

---

I doubt your hypothesis is correct @halter73 because the repro project apparently works Just Fine:tm:. The only difference between good and bad is https://github.com/anvilcloud/HealthChecksRepro/blob/main/src/HealthCheckRepro/Program.cs#L14-L21.#46766

@halter73 could you or someone else on the HealthChecks team please test the reproduction and determine what is actually going on❔

[halter73]

is that the package metadata should already bring in Microsoft.Bcl.AsyncInterfaces when using the netstandard2.0 assembly from this package.

The problem is that the app targets net6.0. Microsoft.Extensions.Diagnostics.HealthChecks 7.0 does not have a net6.0 target while Microsoft.Extensions.Hosting.Abstractions 7.0 has both net6.0 and net7.0 targets. This causes the app to resolve the netstandard2.0 version of HealthChecks that requires Microsoft.Bcl.AsyncInterfaces and the net6.0 version of Hosting.Abstractions which has no transitive dependency to Microsoft.Bcl.AsyncInterfaces as shown by HealthCheckRepro.deps.json:

      "Microsoft.Extensions.Hosting.Abstractions/7.0.0": {
        "dependencies": {
          "Microsoft.Extensions.Configuration.Abstractions": "7.0.0",
          "Microsoft.Extensions.DependencyInjection.Abstractions": "7.0.0",
          "Microsoft.Extensions.FileProviders.Abstractions": "7.0.0"
        },
        "runtime": {
          "lib/net6.0/Microsoft.Extensions.Hosting.Abstractions.dll": {
            "assemblyVersion": "7.0.0.0",
            "fileVersion": "7.0.22.51805"
          }
        }
      },

I doubt your hypothesis is correct @halter73 because the repro project apparently works Just Fine™️. The only difference between good and bad is anvilcloud/HealthChecksRepro@main/src/HealthCheckRepro/Program.cs#L14-L21.#46766

@halter73 could you or someone else on the HealthChecks team please test the reproduction and determine what is actually going on❔

I've already tested the repro. As I mentioned, when you call AddCheck, it hits the following:

aspnetcore/src/HealthChecks/HealthChecks/src/DefaultHealthCheckService.cs

Line 79 in 63a0251

await using (scope.ConfigureAwait(false))

If you do not call AddCheck like in the #if false branch, there is no call to IAsyncDisposable.DisposeAsync so the Microsoft.Bcl.AsyncInterfaces dependency is not required at runtime. But when the netstandard2.0 version of HealthChecks does call DisposeAsync in a net6.0 app without an explicit package reference Microsoft.Bcl.AsyncInterfaces, it breaks with the following exception:

System.IO.FileNotFoundException: 'Could not load file or assembly 'Microsoft.Bcl.AsyncInterfaces, Version=7.0.0.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51'. The system cannot find the file specified.'
   at Microsoft.Extensions.Diagnostics.HealthChecks.DefaultHealthCheckService.RunCheckAsync(HealthCheckRegistration registration, CancellationToken cancellationToken)
   at System.Threading.Tasks.Task`1.InnerInvoke() in /_/src/libraries/System.Private.CoreLib/src/System/Threading/Tasks/Future.cs:line 503

We should be able to automatically detect that there are supported TFMs like net6.0 that do not transitively pull in Microsoft.Bcl.AsyncInterfaces, and fail the build until a direct dependency is added.

[alexperovich]

Thats very interesting. The build never failed because there is no "build" of the net6.0 TFM in the HealthChecks package that could be made to fail, and the netstandard2.0 TFM build pulls in the netstandard2.0 hosting.abstractions which properly has the dependency.

[alexperovich]

I wonder if the Microsoft.Bcl.AsyncInterfaces package could be tweaked to force a top level dependency on any project/TFM that would require it, such that transitive inclusions like this would still work when later referenced with other TFMs.

[dougbu]

@alexperovich @halter73 some of what you said today is new information to me, or at least old information reframed so I can (mostly) get it.

@marcpopMSFT @dsplaisted something complicated and unexpected is occurring during assembly resolution. My flawed intuition says that when I bring in the netstandard2.0 assembly from a package, my project should also bring in the netstandard2.0 assemblies from the packages it references. In this repro, Microsoft.Extensions.Diagnostics.HealthChecks is brought in directly and its netstandard2.0 is chosen. I'm guessing the direct Microsoft.Extensions.Hosting reference is forcing resolution of the net6.0 Hosting.Abstractions assembly. Am I on the right track❔

If yes, there are at least two pertinent questions:

1. What options are available to detect these "mixed band" scenarios❔ @alexperovich has suggested a couple of things but a blanket fix for the packages we ship containing netstandard2.0 assemblies would be a big deal (no matter how small the .targets file would be).
2. Are these supported scenarios❔

/cc @ericstj in case he owns Microsoft.Bcl.AsyncInterfaces or could pull in whoever does.

[dsplaisted]

My flawed intuition says that when I bring in the netstandard2.0 assembly from a package, my project should also bring in the netstandard2.0 assemblies from the packages it references.

No, whether a package is referenced directly or transitively does not affect what target framework is used to select assets. For each package, the assets for the "closest match" of the project's target framework will be used.

I'm not sure about what to do about this. It sounds like Microsoft.Bcl.AsyncInterfaces is a "polyfill" type of library, and that may be something that NuGet / .NET don't handle too well.

[ericstj]

So dependencies of a package are supposed to be part of the public surface area of that package. So it's breaking if a package exposes a dependency on one framework but not on a compatible framework.

The package "at fault" here is Microsoft.Extensions.Hosting.Abstractions. That package included a reference to AsyncInterfaces, but did not include it on frameworks where it is not needed:
https://github.com/dotnet/runtime/blob/b68fd882623b528fd4ef78b122209710f17bacdb/src/libraries/Microsoft.Extensions.Hosting.Abstractions/src/Microsoft.Extensions.Hosting.Abstractions.csproj#L29
Microsoft.Extensions.Hosting.Abstractions was what exposed AsyncInterfaces to HealthChecks.

This has come up before and my recollection was that we intentionally have this gap where we will drop the polyfill packages in newer frameworks because the tradeoff of having the missing dependency that might be needed in some combinations of framework targeting is better than forcing all developers to carry the polyfill even after it's no longer needed. I can't find any of these issues, but I do recall a few. Maybe @joperezr remembers.

As was suggested -- the safe thing to do is for any library that uses the polyfill to directly reference it. I wonder if we could force that by hiding the package from compile (PrivateAssets="Compile") though chances are that would be another case of making it harder for the common case to better handle the edge case.

[dougbu]

Interesting background @ericstj. I don't remember the history but believe ASP.NET has mostly avoided direct package references not required for compilation on a per-framework basis. We certainly have a fair number of conditional references though you need to scroll through https://github.com/dotnet/aspnetcore/search?q=%22condition%3D%5C%22+%27%24%28targetframework%29%22&type=code to find them. I'm not positive how bad that is or how much worse it is in our servicing branches. Sounds like the Extensions packages now in runtime have similar history.

That said, are use of a 7.0.x package when targeting net6.0 or other mixed-band scenarios supported❔ And wouldn't it be a significant breaking change to unconditionally reference polyfill packages everywhere in our servicing branches❔

[mgravell]

That said, are use of a 7.0.x package when targeting net6.0 or other mixed-band scenarios supported❔

I think that's a great question that has never been very clearly stated to the community. The only time I've seen a clear statement here is in System.Collections.Immutable, where v7 includes (for anything below net6)

<Warning Text="System.Collections.Immutable 7.0.0 doesn't support $(TargetFramework) and has not been tested with it. Consider upgrading your TargetFramework to net6.0 or later. You may also set &lt;SuppressTfmSupportBuildWarnings&gt;true&lt;/SuppressTfmSupportBuildWarnings&gt; in the project file to ignore this warning and attempt to run in this unsupported configuration at your own risk." />

Most other things: don't complain, so I believe people have rightly or wrongly thought "that's fine". A very clear statement would be good, to help decide the route.

Some options (just trying to keep driving this forward, as part of build-ops)

1. add a warning like the above?
2. remove the net6 TFM from Microsoft.Extensions.Hosting.Abstractions
3. add a net6 TFM to Microsoft.Extensions.Diagnostics.HealthChecks with a comment pointing to the problem
4. do nothing (except perhaps issue guidance)
5. add an explicit Microsoft.Bcl.AsyncInterfaces ref to the ns2.0 version of Microsoft.Extensions.Diagnostics.HealthChecks
6. something else

thoughts? (personal subjective opinion; 5 seems low impact - doesn't confuse unnecessary TFMs, and the shim should evaporate if it turns out to be unnecessary)

[mgravell]

As a side note: if the official story is "don't go above your TFM major", that sounds like something where the NuGet/package tooling could do a lot to help out (with some new "I'm part of the runtime, respect my TFM major" flag), because right now it will blindly suggest updates spanning majors, but won't suggest minors in the same major that exist in that scenario; i.e.

• project targets net6.0
• package ref to sys-package 6.0.1
• nuget has 6.0.7 and 7.0.1
• tooling will suggest 7.0.1 but not 6.0.7

[ericstj]

We permit use of packages from newer releases on older frameworks. There are consequences / trade-offs / side-effects of mixing bands, but it is supported so long as you do not downgrade. Good examples of this are packages like System.Text.Json - we explicitly put new features and API in this package so that folks can use it on .NETStandard / .NETFramework (and older .NET versions).

Some of the consequences of using newer packages on older frameworks:

1. Your application will carry more dependencies with it instead of using the version in the framework. This can result in a larger app size.
2. Similar to 1, when carrying the dependency in your app you're responsible for servicing it. For example: if you use the System.Text.Json in the framework you target you don't need to update your app when the library updates. You can depend on the framework updating it. If you use the package from a newer framework, you need to update your app when new versions of the package ship with important updates.
3. The adhoc combination of packages could result in situations that have less test coverage. You might find issues that we didn't catch during the product development cycle. Those issues will need to be brought to servicing which has a higher bar.
4. It's possible for the newer band to go out of support before the older framework which would force the app to move to an even newer band when the previous one went out of support. This happened with 3.1 / 5.0. 5.0 went out of support before 3.1, so an app might want to stay on 3.1 but would need to update a package to 6.0 to keep getting updates.

Adding PackageReferences for anything that's directly consumed by your assembly is actually a best practice since it insulates you from changes made by your dependencies. You can know you will need that PackageReference because it appears as an assembly dependency in your metadata. That won't change unless you rebuild the library.
I've heard customers actually try to enforce "absolute" direct references through analyzers to achieve this: remove any direct references that aren't directly used and promote indirect references to direct references if they are directly used. Some see this as undesirable since it adds more references to the project file, while others see this as desirable because it provides the accurate representation of every dependency of the library.

In other words, I recommend option 5 as well. It's not only low risk, but it's also best practice IMO.

[dougbu]

5, add an explicit Microsoft.Bcl.AsyncInterfaces ref to the ns2.0 version of Microsoft.Extensions.Diagnostics.HealthChecks

In other words, I recommend option 5 as well. It's not only low risk, but it's also best practice IMO.

This makes sense. @mgravell do you want to take this as part of build ops❔ My main added suggestion is that we run the change by Tactics since it's a packaging change (at least kinda sorta).

[dougbu]

My main added suggestion is that we run the change by Tactics …

To be clear, I'm assuming we'll take this back to at least release/7.0.

[dougbu]

@ericstj what is the list of "polyfill packages" we should be concerned about❔ I'm thinking it might be worthwhile to expand @MackinnonBuck's #46920 to cover all the places where these packages show up in project.assets.json or *.deps.json files but not the relevant *.csproj file.

Also, which of project.assets.json and *.deps.json is the better place to look for the final set of package references❔

[ericstj]

what is the list of "polyfill packages" we should be concerned about

Microsoft.BCL.*

If you wanted a good solution to avoid this entire class of issues in the future, it would be to implement "Package Testing". We do this in dotnet/runtime. For every package we produce, we determine what frameworks it supports. We then restore that package on those frameworks and walk its closure of dependencies to ensure that all dependencies are present.
https://github.com/dotnet/runtime/tree/main/src/libraries/testPackages
If you'd like we can give a demo of this functionality and see if there is a way for ASP.NET to adopt it.
cc @ViktorHofer @joperezr

[ghost]

We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.