Microsoft.AspNetCore.App.runtimeconfig.json specifies runtime properties which values should not bet set by a shared framework

[vitek-karas]

This is how the Microsoft.AspNetCore.App.runtimeconfig.json file looks like in a recent P7 build:

{
  "runtimeOptions": {
    "tfm": "net8.0",
    "framework": {
      "name": "Microsoft.NETCore.App",
      "version": "8.0.0-preview.7.23359.3"
    },
    "rollForward": "LatestPatch",
    "configProperties": {
      "System.Runtime.Loader.UseRidGraph": true,
      "System.Reflection.Metadata.MetadataUpdater.IsSupported": false,
      "System.Runtime.Serialization.EnableUnsafeBinaryFormatterSerialization": false
    }
  }
}

The configProperties section should not be there. They got there via the process used to generate the shared framework. ASP.NET builds what looks like a framework dependent console app and uses the output to construct the shared framework. The MetadataUpdater and BinaryFormatSerializer properties are defaults set by the SDK for console apps. They may differ for other types of apps and ASP.NET shared framework should not dictate the values on its own, it's the job of the SDK to specify the defaults (and let the user override those). The UseRidGraph is even worse, that one comes from the workaround used in the ASP.NET repo itself introduced here: https://github.com/dotnet/aspnetcore/pull/48908#issuecomment-1601894643. It must not be there when we ship.

Both runtime and WindowsDesktop had a similar issue for a long time now, but that has been fixed in https://github.com/dotnet/arcade/pull/13844. Unfortunately ASP.NET doesn't use Arcade to build the shared framework (unlike runtime and WindowsDesktop) - see https://github.com/dotnet/aspnetcore/issues/48013.

We need to fix at least the UseRidGraph before we ship, but ideally all of the properties should be removed.

/fyi @elinor-fung @agocke

[mkArtakMSFT]

@vitek-karas thanks for bringing this up. What do you think the customer impact be here? Also, if we remove the configProperties section, will those settings / properties get the same values or will they get potentially other values and what would be the impact of that?

[vitek-karas]

The impact is probably not that widespread but it makes for really weird experiences when it happens. See https://github.com/dotnet/sdk/issues/32941#issuecomment-1571102871 - this happened because of the same bug in the runtime. We had to introduce a workaround, but it's tricky and potentially unreliable (ordering and such).

The UseRidGraph is something we really don't want - we made an intentional breaking change in .NET 8 to move away from the super complicated RID graph and its maintenance. This knob is to explicitly ask for a backward compat behavior, but we obviously want as many people as possible on the new behavior. If ASP.NET silently moves everybody onto the backward compat behavior that whole effort is seriously undermined.

The binary formatter setting effectively defeats part of the logic in the SDK: https://github.com/dotnet/sdk/blob/52d9ab4ce69f2f134e582f2dc449594b8e4d0bfc/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.targets#L141-L178 - it should be that logic which decides what the value should be and if it should actually set the runtime property. ASP.NET should not be setting any default value for it (which is effectively what happens here).

The MetadataUpdater property is one which we actually shipped with even in .NET 6 - unfortunately. But otherwise it's the same story - SDK has logic to set this value here https://github.com/dotnet/sdk/blob/52d9ab4ce69f2f134e582f2dc449594b8e4d0bfc/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.targets#L133, and that should be making the decisions, not the framework.

The MetadataUpdater is probably the least significant because the SDK will set it to true when it's really needed (HotReload) otherwise it can be off. And so us shipping that in .NET 6+ doesn't hurt that much. But the other two which are added now in 8 are more problematic and they would have different values without the framework setting them like this.

[mkArtakMSFT]

@vitek-karas my only concern is that it feels a bit risky to do this at this stage in the release. If you think this is critical, I think we should bring this up to in Tactics and discuss there. @danmoseley FYI

[vitek-karas]

I think the UseRifGraph and EnableUnsafeBinaryFormatterSerialization should be fixed before we ship .NET 8. They are not there in .NET 7 and would be new in .NET 8.

The RID one is basically degrading the value of a rather difficult breaking change and delaying its effectiveness one more release. @richlander @elinor-fung - how do you feel if ASP.NET framework forced all ASP.NET apps to rely on the backward compat RID graph behavior in .NET 8?

The binary formatter required a workaround in the SDK, which works but is relatively fragile and it's unclear if there are other verticals which will be hurt by the same problem as WinForms.

If you're concerned about risk, then we can keep the MetadataUpdater property there since we've shipped with it in 6 and 7.

[elinor-fung]

I think the UseRifGraph and EnableUnsafeBinaryFormatterSerialization should be fixed before we ship .NET 8. They are not there in .NET 7 and would be new in .NET 8.

Agreed - at least the new-in-8 ones should be fixed.

The RID one is basically degrading the value of a rather difficult breaking change and delaying its effectiveness one more release. @richlander @elinor-fung - how do you feel if ASP.NET framework forced all ASP.NET apps to rely on the backward compat RID graph behavior in .NET 8?

I very much do not think the framework should make all ASP.NET use the backwards compat switch.

[mkArtakMSFT]

Sound good, thanks for additional context @vitek-karas and for your feedback too, @elinor-fung. @wtgodbe can you please try to implement this and hopefully nothing will break. And if it does, we'll react afterwards.

[wtgodbe]

https://github.com/dotnet/aspnetcore/pull/49682 removed EnableUnsafeBinaryFormatterSerialization and UseRidGraph. Keeping this open to track removing MetadataUpdater in 9.0