Make non-file URL support optional in XmlUrlResolver

[MichalStrehovsky]

Make non-file URL support optional in System.Xml

Currently some root-level XML APIs take URI as string as an input (type is string but they contain Uri in the name, i.e. inputUri and similar). Typically those APIs are used with file paths but also support URLs. By default URI handling for those APIs is handled by XmlUrlResolver. We have changed XML to not use XmlUrlResolver in many places already (i.e. some XML constructs allow specyfing links, i.e. DTD, schema) - we have switched them to use null resolver making this forbidden. Root level APIs are the only place we're still using XmlUrlResolver by default because it was a trade-off at the moment - since user explicitly asks to load something we decided that behavior was acceptable.

Per original request (in the bottom of this proposal) in trimming scenarios that causes us to pull in networking dependency during trimming causing those relatively rarer scenarios (path is the most common scenario) to increase size by around 3MB.

Proposal is to have default resolver controllable with switch which allows or not URLs (pick different resolver).

Usage

For trimming scenarios we will not resolve URLs by default - only file system resolving will be supported. To opt-in set System.Xml.XmlResolver.IsNetworkingEnabledByDefault to true with RuntimeHostConfigurationOption in your csproj.

For non-trimming scenarios switch is opt-out and URL resolution needs to be turned off (in concern for backward compatibility).

AppContext.SetSwitch("System.Xml.XmlResolver.IsNetworkingEnabledByDefault", false);

API proposal

Add following switch (other switches start with Switch.):

System.Xml.XmlResolver.IsNetworkingEnabledByDefault

with value true by default for non-trimming scenarios and false for trimming.

Add public API with new resolver - while this is not strictly necessary to unblock reported scenario it complements it and allows to not access network (and good trimming) also in other scenarios which are very common (i.e. validating XML with schema with references in the same folder etc.).

namespace System.Xml;

public abstract partial class XmlResolver
{
// Existing:
// public static System.Xml.XmlResolver ThrowingResolver { get; }
public static System.Xml.XmlResolver FileSystemResolver { get; }
// also note: replacement for XmlUrlResolver which is separate public class
}

Original request (by @MichalStrehovsky)

We should consider a feature switch port of dotnet/corefx#31510.

Experimentally stubbing out XmlDownloadManager.GetNonFileStreamAsync I get these results with Native AOT for a simple app that just does XDocument.Load("");:

Baseline: 7,531,520 bytes
Stubbed: 3,795,456 bytes

We would have a use case for it in crossgen2 and ILC because both use XML, but neither require non-filestream support. This currently drags in the entire network stack for no reason.

[ghost]

Tagging subscribers to this area: @dotnet/area-system-xml
See info in area-owners.md if you want to be subscribed.

Issue Details

---

We should consider a feature switch port of dotnet/corefx#31510.

Experimentally stubbing out XmlDownloadManager.GetNonFileStreamAsync I get these results with Native AOT for a simple app that just does XDocument.Load("");:

Baseline: 7,531,520 bytes
Stubbed: 3,795,456 bytes

We would have a use case for it in crossgen2 and ILC because both use XML, but neither require non-filestream support. This currently drags in the entire network stack for no reason.

Author:	MichalStrehovsky
Assignees:	-
Labels:	area-System.Xml, linkable-framework
Milestone:	-

[ghost]

Tagging subscribers to 'linkable-framework': @eerhardt, @vitek-karas, @LakshanF, @sbomer, @joperezr
See info in area-owners.md if you want to be subscribed.

Issue Details

---

We should consider a feature switch port of dotnet/corefx#31510.

Experimentally stubbing out XmlDownloadManager.GetNonFileStreamAsync I get these results with Native AOT for a simple app that just does XDocument.Load("");:

Baseline: 7,531,520 bytes
Stubbed: 3,795,456 bytes

We would have a use case for it in crossgen2 and ILC because both use XML, but neither require non-filestream support. This currently drags in the entire network stack for no reason.

Author:	MichalStrehovsky
Assignees:	-
Labels:	area-System.Xml, untriaged, linkable-framework
Milestone:	-

[eerhardt]

Alternatively, what if we added a new API that didn’t do web requests and then we didn’t need a feature switch?

‘XDocument.LoadFromFile’

[stephentoub]

Alternatively, what if we added a new API that didn’t do web requests and then we didn’t need a feature switch?

‘XDocument.LoadFromFile’

If that worked, I'd be in favor of it; the less we need to rely on feature switches, the better. But unfortunately I don't think it's that simple. I expect you'll find that any use of XmlReaders, regardless of how they're constructed, will end up rooting that GetNonFileStreamAsync method. Someone would need to prototype reworking the innards to remove those choke points.

[MichalStrehovsky]

Yes, the web stack is included even if the Stream overload of the constructor is called.

[krwq]

I've created a draft PR with suggested approach: #84169

The approach is adding new resolver which resolves URIs with file scheme (accessible with XmlResolver.FileSystemResolver) and switch which changes so that we use it rather than XmlUrlResolver. In most situation we never do any resolving for XMLs but XmlUrlResolver is still used to resolve URIs for root level APIs, the switch changes so that we use file resolver instead and never access the network

[terrajobst]

Video

• Let's do both:
  1. Let's investigate if we can refactor XML so that non-URL based use doesn't result in static dependencies of networking
  2. Let's add a big switch that allows to turn off all networking from XML
• The default behavior
  • It's on by default for now
  • We should look into making it a breaking change to not allow networking by default, but we shouldn't tie it to trimming
  • This would be an extension of what we already did (i.e. not resolving DTDs)
• Name:
  • We prefer IsNetworkingEnabledByDefault
  • We should name it like we'd name an API, which means we'd need a type were we'd put it
  • System.Xml.XmlResolver.IsNetworkingEnabledByDefault

[MichalStrehovsky]

Reopening to track SDK and docs work (feel free to close if this is tracked elsewhere and sorry for the noise in that case). This would be hard/impossible to use without that:

• We should add a property people can use to opt out. The necessary change on the SDK side will look something like this: https://github.com/dotnet/sdk/blob/7059317381a130ea7ee5e48a76b5c28a7bb7d919/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.targets#L490-L493
• We should document the new property. That looks like this: Document StackTraceSupport property docs#36080

[eiriktsarpalis]

Reopening to track SDK and docs work

It's unlikely this will be actioned upon unless follow-up issues are opened in the relevant repos. Closing this one and opening new issues there.

[MichalStrehovsky]

Reopening to track SDK and docs work

It's unlikely this will be actioned upon unless follow-up issues are opened in the relevant repos. Closing this one and opening new issues there.

If this is to get the issue off of the team's 8.0 query, it accomplishes it. But note that adding docs+exposing this from the SDK side has been traditionally done by the feature team.

Filing issues in the docs/sdk repo is usually a pointless exercise because if you look at the repo history, there are many issues going years without triage/milestone. It is the issue graveyard and nobody is on point to fix these for you. I don't believe they are included in any product queries. So if the Xml team actually wants customers to benefit from this, I would suggest tracking it as 8.0 here.

[eiriktsarpalis]

But note that adding docs+exposing this from the SDK side has been traditionally done by the feature team.

Filing issues on the docs repo is something that has worked for us in the past, even in cases where the changes were being made by the area owners.

Filing issues in the docs/sdk repo is usually a pointless exercise because if you look at the repo history, there are many issues going years without triage/milestone. It is the issue graveyard and nobody is on point to fix these for you.

Fair. I'll move that issue back to runtime but no promises we'll get this done in time for RC1.