Remove System.Net.Requests dependency from System.Private.Xml

[stephentoub]

Anything that uses System.Xml ends up implicitly referencing this System.Net.Requests.dll, which in a trimmed default MVC app is 97K (plus, System.Net.Requests.dll in turn references a whole bunch of stuff that's otherwise unused). The only thing it's used for is as part of XmlResolver to download the specified url. We can instead remove the usage of WebRequest.Create and replace it with usage of HttpClient, which is already brought in because System.Net.Requests uses it to implement HttpWebRequest.

@krwq, @buyaa-n, there are two breaking changes here (which is also why I've temporarily marked this as no-merge, until we can discuss it appropriately):

• Previously you could have specified a url with a scheme other than file, http, or https, and it may have worked. To my knowledge the only other scheme that had built-in support was ftp, but you could also use WebRequest.RegisterPrefix to register a custom scheme handler, and then it seems that this XmlDownloadManager would have been able to use it. How important is it to keep this functionality? Have you ever seen or heard of someone using it with XmlResolver?
• Because we're now using HttpClient instead of HttpWebRequest, download failures that would have previously thrown WebException will now throw HttpRequestException. How do we feel about that? If you think it'll be impactful, we could push System.Net.WebException down to a lower-assembly, like System.Net.Primitives, but that'd be a bit unfortunate.

cc: @jkotas, @davidsh, @danmosemsft

[davidsh]

If you think it'll be impactful, we could push System.Net.WebRequest down to a lower-assembly, like System.Net.Primitives, but that'd be a bit unfortunate.

"System.Net.WebRequest" - What is that? Maybe you meant to say "push WebException to a lower-assembly"?

I don't think we want the WebRequest family of APIs (which are considered the 'legacy' APIs) pushed into System.Net.Primitives.

[stephentoub]

"System.Net.WebRequest" - What is that? Maybe you meant to say "push WebException to a lower-assembly"?

Typo, I meant WebException. Fixed.

[krwq]

I'm fine with the breaking change but we need to make sure it's documented later down the cycle and since already labeled as such then plan sounds good to me (haven't looked at the changeset yet).

[danmoseley]

@ericstj how should we record breaking changes for 5.0? Label on PR?

[buyaa-n]

Previously you could have specified a url with a scheme other than file, http, or https, and it may have worked. To my knowledge the only other scheme that had built-in support was ftp, but you could also use WebRequest.RegisterPrefix to register a custom scheme handler, and then it seems that this XmlDownloadManager would have been able to use it. How important is it to keep this functionality? Have you ever seen or heard of someone using it with XmlResolver?

I cannot say how much it is used for downloading ftp resource, but seems it is used some level https://forums.asp.net/t/1378012.aspx?Using+XDocument+Load+to+read+a+file+from+an+FTP+address

XmlUrlResolver res = new XmlUrlResolver();
res.Credentials = new NetworkCredential("username", "password");
XmlReaderSettings set = new XmlReaderSettings();
set.XmlResolver = res;
XmlReader reader  = XmlReader.Create("ftp://host.test", set));

Looks this is the only method for downloading external resource of XmlUrlResolver so XmlReader.Create("ftp://host.test", set) would eventually call/use this method of XmlDownloadManager. So I assume XmlDownloadManager.GetStream(...) should keep supporting ftp.

[krwq]

Note that this is from 2009 and since then new apps (I believe) are mostly using JSON. Also we won't change full framework behavior and to workaround the problem customer can create a stream themselves and pass it to XmlReader or for more advanced scenarios create a custom XmlResolver which did what it used to (we can even put the implementation in the sample or breaking change docs if needed).

I wouldn't keep dependency to FTP given the workarounds and combined with the fact that we discourage people from using XmlResolver completely for security reasons.

[ericstj]

@ericstj how should we record breaking changes for 5.0? Label on PR?

Yeah, then we'll need to generate docs from those PRs by the time we ship.

[stephentoub]

Yeah, then we'll need to generate docs from those PRs by the time we ship.

Is there some place I should proactively be adding content to, other than adding the breaking change label (which I've already done)?

Thanks.